Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PG17 compatibility: Resolve compilation issues #7651

Closed
wants to merge 21 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
ae3ed7d
Enable configure
naisila Jul 8, 2024
3f5a1e0
Rename foreach_ macros to foreach_declared_ macros
naisila Jul 8, 2024
a816d1b
Remove ExecFreeExprContext call
naisila Jul 8, 2024
646ff0a
PG17 uses streaming IO in analyze, fix scan_analyze_next_block function
naisila Jul 8, 2024
85a3ed0
Define ObjectClass for PG17+ only since it's removed
naisila Jul 8, 2024
2434eff
No new node-wide object added so far in PG17, change assert
naisila Jul 8, 2024
2076339
Remove ReorderBufferTupleBuf structure.
naisila Jul 8, 2024
e9fe257
Define colliculocale and daticulocale since they have been renamed
naisila Jul 8, 2024
e33736e
makeStringConst defined in PG17
naisila Jul 8, 2024
4eafd3a
RangeVarCallbackOwnsTable was replaced by RangeVarCallbackMaintainsTable
naisila Jul 9, 2024
be2bec0
attstattarget is nullable, define pg compatible functions for it
naisila Jul 9, 2024
0176974
stxstattarget is nullable in PG17, write compat functions for it
naisila Jul 9, 2024
dd19847
Use ResourceOwner to track WaitEventSet in PG17
naisila Jul 10, 2024
022ca99
getIdentitySequence now uses Relation instead of relation_id
naisila Jul 10, 2024
e0a0874
Remove no-op tuplestore_donestoring function
naisila Jul 10, 2024
49f7bff
MergeAction can have 3 merge kinds (now enum) in PG17, write compat
naisila Jul 10, 2024
a23bf88
EXPLAIN (MEMORY) is added, make changes to ExplainOnePlan
naisila Jul 10, 2024
651bcc7
LIMIT_OPTION_DEFAULT has been removed as it's useless, use LIMIT_OPTI…
naisila Jul 10, 2024
ec9beb5
write compat for create_foreignscan_path bcs of more arguments in PG17
naisila Jul 10, 2024
140a443
pgprocno and lxid have been combined into a struct in PGPROC
naisila Jul 10, 2024
76f60a7
add pg17 build test
naisila Jul 25, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions .devcontainer/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,18 @@ RUN mkdir .pgenv-staging/
RUN cp -r .pgenv/src .pgenv/pgsql-* .pgenv/config .pgenv-staging/
RUN rm .pgenv-staging/config/default.conf

FROM base AS pg17
RUN MAKEFLAGS="-j $(nproc)" pgenv build 17beta2
RUN rm .pgenv/src/*.tar*
RUN make -C .pgenv/src/postgresql-*/ clean
RUN make -C .pgenv/src/postgresql-*/src/include install

# create a staging directory with all files we want to copy from our pgenv build
# we will copy the contents of the staged folder into the final image at once
RUN mkdir .pgenv-staging/
RUN cp -r .pgenv/src .pgenv/pgsql-* .pgenv/config .pgenv-staging/
RUN rm .pgenv-staging/config/default.conf

FROM base AS uncrustify-builder

RUN sudo apt update && sudo apt install -y cmake tree
Expand Down
6 changes: 4 additions & 2 deletions .github/workflows/build_and_test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,12 @@ jobs:
style_checker_image_name: "ghcr.io/citusdata/stylechecker"
style_checker_tools_version: "0.8.18"
sql_snapshot_pg_version: "16.3"
image_suffix: "-v13fd57c"
image_suffix: "-dev-16d4616"
pg14_version: '{ "major": "14", "full": "14.12" }'
pg15_version: '{ "major": "15", "full": "15.7" }'
pg16_version: '{ "major": "16", "full": "16.3" }'
upgrade_pg_versions: "14.12-15.7-16.3"
pg17_version: '{ "major": "17", "full": "17beta2" }'
upgrade_pg_versions: "14.12-15.7-16.3-17beta2"
steps:
# Since GHA jobs needs at least one step we use a noop step here.
- name: Set up parameters
Expand Down Expand Up @@ -113,6 +114,7 @@ jobs:
- ${{ needs.params.outputs.pg14_version }}
- ${{ needs.params.outputs.pg15_version }}
- ${{ needs.params.outputs.pg16_version }}
- ${{ needs.params.outputs.pg17_version }}
runs-on: ubuntu-20.04
container:
image: "${{ matrix.image_name }}:${{ fromJson(matrix.pg_version).full }}${{ matrix.image_suffix }}"
Expand Down
2 changes: 1 addition & 1 deletion configure
Original file line number Diff line number Diff line change
Expand Up @@ -2588,7 +2588,7 @@ fi
if test "$with_pg_version_check" = no; then
{ $as_echo "$as_me:${as_lineno-$LINENO}: building against PostgreSQL $version_num (skipped compatibility check)" >&5
$as_echo "$as_me: building against PostgreSQL $version_num (skipped compatibility check)" >&6;}
elif test "$version_num" != '14' -a "$version_num" != '15' -a "$version_num" != '16'; then
elif test "$version_num" != '14' -a "$version_num" != '15' -a "$version_num" != '16' -a "$version_num" != '17'; then
as_fn_error $? "Citus is not compatible with the detected PostgreSQL version ${version_num}." "$LINENO" 5
else
{ $as_echo "$as_me:${as_lineno-$LINENO}: building against PostgreSQL $version_num" >&5
Expand Down
2 changes: 1 addition & 1 deletion configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ AC_SUBST(with_pg_version_check)

if test "$with_pg_version_check" = no; then
AC_MSG_NOTICE([building against PostgreSQL $version_num (skipped compatibility check)])
elif test "$version_num" != '14' -a "$version_num" != '15' -a "$version_num" != '16'; then
elif test "$version_num" != '14' -a "$version_num" != '15' -a "$version_num" != '16' -a "$version_num" != '17'; then
AC_MSG_ERROR([Citus is not compatible with the detected PostgreSQL version ${version_num}.])
else
AC_MSG_NOTICE([building against PostgreSQL $version_num])
Expand Down
15 changes: 5 additions & 10 deletions src/backend/columnar/columnar_customscan.c
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you consider using the macros from pg17 (e.g. removing the local variables)? Would it be feasible?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still have to support PG15 and PG16. One way to do it would be to change Citus's current definition of these macros such that they are identical to PG17's definitions, and then iterate over all uses and remove the local variables.
Do you think that is better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would require to add the macro definition for PG < 17. Both (the current / alternative) seem feasible options, and both require quite some editing. You already implemented one of them. It seems OK to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@naisila, personally I think that using PG17's definitions when PG_VERSION_NUM >= PG_VERSION_17 and having identical definitions for PG15 and PG16 is better for you will naturally transition to PostgreSQL's macros when you no longer have to support PG15 and PG16. If you keep your own macros, you'll still have to support them even when PG15 and PG16 are no longer supported, or replace them with PostgreSQL's macros in the future.
Anyway, that's just my opinion, I'm not the one who'll have to support Citus, so it's not my call.

Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ ColumnarGetRelationInfoHook(PlannerInfo *root, Oid relationObjectId,

/* disable index-only scan */
IndexOptInfo *indexOptInfo = NULL;
foreach_ptr(indexOptInfo, rel->indexlist)
foreach_declared_ptr(indexOptInfo, rel->indexlist)
{
memset(indexOptInfo->canreturn, false, indexOptInfo->ncolumns * sizeof(bool));
}
Expand All @@ -381,7 +381,7 @@ RemovePathsByPredicate(RelOptInfo *rel, PathPredicate removePathPredicate)
List *filteredPathList = NIL;

Path *path = NULL;
foreach_ptr(path, rel->pathlist)
foreach_declared_ptr(path, rel->pathlist)
{
if (!removePathPredicate(path))
{
Expand Down Expand Up @@ -428,7 +428,7 @@ static void
CostColumnarPaths(PlannerInfo *root, RelOptInfo *rel, Oid relationId)
{
Path *path = NULL;
foreach_ptr(path, rel->pathlist)
foreach_declared_ptr(path, rel->pathlist)
{
if (IsA(path, IndexPath))
{
Expand Down Expand Up @@ -783,7 +783,7 @@ ExtractPushdownClause(PlannerInfo *root, RelOptInfo *rel, Node *node)
List *pushdownableArgs = NIL;

Node *boolExprArg = NULL;
foreach_ptr(boolExprArg, boolExpr->args)
foreach_declared_ptr(boolExprArg, boolExpr->args)
{
Expr *pushdownableArg = ExtractPushdownClause(root, rel,
(Node *) boolExprArg);
Expand Down Expand Up @@ -1550,7 +1550,7 @@ ColumnarPerStripeScanCost(RelOptInfo *rel, Oid relationId, int numberOfColumnsRe
uint32 maxColumnCount = 0;
uint64 totalStripeSize = 0;
StripeMetadata *stripeMetadata = NULL;
foreach_ptr(stripeMetadata, stripeList)
foreach_declared_ptr(stripeMetadata, stripeList)
{
totalStripeSize += stripeMetadata->dataLength;
maxColumnCount = Max(maxColumnCount, stripeMetadata->columnCount);
Expand Down Expand Up @@ -1924,11 +1924,6 @@ ColumnarScan_EndCustomScan(CustomScanState *node)
*/
TableScanDesc scanDesc = node->ss.ss_currentScanDesc;

/*
* Free the exprcontext
*/
ExecFreeExprContext(&node->ss.ps);

/*
* clean out the tuple table
*/
Expand Down
2 changes: 1 addition & 1 deletion src/backend/columnar/columnar_metadata.c
Original file line number Diff line number Diff line change
Expand Up @@ -2041,7 +2041,7 @@
List *stripeMetadataList = ReadDataFileStripeList(storageId,
GetTransactionSnapshot());
StripeMetadata *stripeMetadata = NULL;
foreach_ptr(stripeMetadata, stripeMetadataList)
foreach_declared_ptr(stripeMetadata, stripeMetadataList)

Check warning on line 2044 in src/backend/columnar/columnar_metadata.c

View check run for this annotation

Codecov / codecov/patch

src/backend/columnar/columnar_metadata.c#L2044

Added line #L2044 was not covered by tests
{
highestRowNumber = Max(highestRowNumber,
StripeGetHighestRowNumber(stripeMetadata));
Expand Down
4 changes: 2 additions & 2 deletions src/backend/columnar/columnar_reader.c
Original file line number Diff line number Diff line change
Expand Up @@ -880,7 +880,7 @@ ReadChunkGroupNextRow(ChunkGroupReadState *chunkGroupReadState, Datum *columnVal
memset(columnNulls, true, sizeof(bool) * chunkGroupReadState->columnCount);

int attno;
foreach_int(attno, chunkGroupReadState->projectedColumnList)
foreach_declared_int(attno, chunkGroupReadState->projectedColumnList)
{
const ChunkData *chunkGroupData = chunkGroupReadState->chunkGroupData;
const int rowIndex = chunkGroupReadState->currentRow;
Expand Down Expand Up @@ -1489,7 +1489,7 @@ ProjectedColumnMask(uint32 columnCount, List *projectedColumnList)
bool *projectedColumnMask = palloc0(columnCount * sizeof(bool));
int attno;

foreach_int(attno, projectedColumnList)
foreach_declared_int(attno, projectedColumnList)
{
/* attno is 1-indexed; projectedColumnMask is 0-indexed */
int columnIndex = attno - 1;
Expand Down
9 changes: 7 additions & 2 deletions src/backend/columnar/columnar_tableam.c
Original file line number Diff line number Diff line change
Expand Up @@ -1424,8 +1424,13 @@


static bool
columnar_scan_analyze_next_block(TableScanDesc scan, BlockNumber blockno,
columnar_scan_analyze_next_block(TableScanDesc scan,
#if PG_VERSION_NUM >= PG_VERSION_17
ReadStream *stream)
#else
BlockNumber blockno,
BufferAccessStrategy bstrategy)
#endif
{
/*
* Our access method is not pages based, i.e. tuples are not confined
Expand Down Expand Up @@ -3083,7 +3088,7 @@
GetExtensionOption(List *extensionOptions, const char *defname)
{
DefElem *defElement = NULL;
foreach_ptr(defElement, extensionOptions)
foreach_declared_ptr(defElement, extensionOptions)

Check warning on line 3091 in src/backend/columnar/columnar_tableam.c

View check run for this annotation

Codecov / codecov/patch

src/backend/columnar/columnar_tableam.c#L3091

Added line #L3091 was not covered by tests
{
if (IsA(defElement, DefElem) &&
strncmp(defElement->defname, defname, NAMEDATALEN) == 0)
Expand Down
71 changes: 71 additions & 0 deletions src/backend/distributed/cdc/cdc_decoder.c
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
#include "utils/rel.h"
#include "utils/typcache.h"

#include "pg_version_constants.h"

PG_MODULE_MAGIC;

extern void _PG_output_plugin_init(OutputPluginCallbacks *cb);
Expand Down Expand Up @@ -435,6 +437,74 @@ TranslateChangesIfSchemaChanged(Relation sourceRelation, Relation targetRelation
return;
}

#if PG_VERSION_NUM >= PG_VERSION_17

/* Check the ReorderBufferChange's action type and handle them accordingly.*/
switch (change->action)
{
case REORDER_BUFFER_CHANGE_INSERT:
{
/* For insert action, only new tuple should always be translated*/
HeapTuple sourceRelationNewTuple = change->data.tp.newtuple;
HeapTuple targetRelationNewTuple = GetTupleForTargetSchemaForCdc(
sourceRelationNewTuple, sourceRelationDesc, targetRelationDesc);
change->data.tp.newtuple = targetRelationNewTuple;
break;
}

/*
* For update changes both old and new tuples need to be translated for target relation
* if the REPLICA IDENTITY is set to FULL. Otherwise, only the new tuple needs to be
* translated for target relation.
*/
case REORDER_BUFFER_CHANGE_UPDATE:
{
/* For update action, new tuple should always be translated*/
/* Get the new tuple from the ReorderBufferChange, and translate it to target relation. */
HeapTuple sourceRelationNewTuple = change->data.tp.newtuple;
HeapTuple targetRelationNewTuple = GetTupleForTargetSchemaForCdc(
sourceRelationNewTuple, sourceRelationDesc, targetRelationDesc);
change->data.tp.newtuple = targetRelationNewTuple;

/*
* Format oldtuple according to the target relation. If the column values of replica
* identiy change, then the old tuple is non-null and needs to be formatted according
* to the target relation schema.
*/
if (change->data.tp.oldtuple != NULL)
{
HeapTuple sourceRelationOldTuple = change->data.tp.oldtuple;
HeapTuple targetRelationOldTuple = GetTupleForTargetSchemaForCdc(
sourceRelationOldTuple,
sourceRelationDesc,
targetRelationDesc);

change->data.tp.oldtuple = targetRelationOldTuple;
}
break;
}

case REORDER_BUFFER_CHANGE_DELETE:
{
/* For delete action, only old tuple should be translated*/
HeapTuple sourceRelationOldTuple = change->data.tp.oldtuple;
HeapTuple targetRelationOldTuple = GetTupleForTargetSchemaForCdc(
sourceRelationOldTuple,
sourceRelationDesc,
targetRelationDesc);

change->data.tp.oldtuple = targetRelationOldTuple;
break;
}

default:
{
/* Do nothing for other action types. */
break;
}
}
#else

/* Check the ReorderBufferChange's action type and handle them accordingly.*/
switch (change->action)
{
Expand Down Expand Up @@ -499,4 +569,5 @@ TranslateChangesIfSchemaChanged(Relation sourceRelation, Relation targetRelation
break;
}
}
#endif
}
4 changes: 2 additions & 2 deletions src/backend/distributed/clock/causal_clock.c
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ GetHighestClockInTransaction(List *nodeConnectionList)
{
MultiConnection *connection = NULL;

foreach_ptr(connection, nodeConnectionList)
foreach_declared_ptr(connection, nodeConnectionList)
{
int querySent =
SendRemoteCommand(connection, "SELECT citus_get_node_clock();");
Expand All @@ -349,7 +349,7 @@ GetHighestClockInTransaction(List *nodeConnectionList)
globalClockValue->counter)));

/* fetch the results and pick the highest clock value of all the nodes */
foreach_ptr(connection, nodeConnectionList)
foreach_declared_ptr(connection, nodeConnectionList)
{
bool raiseInterrupts = true;

Expand Down
Loading
Loading