agora inbox for [email protected]help / color / mirror / Atom feed
[PATCH] Restructure repack worker teardown 41+ messages / 2 participants [nested] [flat]
* [PATCH] Restructure repack worker teardown @ 2026-05-18 17:13 Álvaro Herrera <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Álvaro Herrera @ 2026-05-18 17:13 UTC (permalink / raw) The original code would leave a shared memory segment unreleased if we fail partway through initialization. Change the shutdown order so that we already free it. Author: Álvaro Herrera <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/backend/commands/repack.c | 67 ++++++++++++++++------------------- 1 file changed, 31 insertions(+), 36 deletions(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index bfc62c8f752..c9064d8fd13 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -3411,10 +3411,14 @@ start_repack_decoding_worker(Oid relid) shm_mq_handle *mqh; BackgroundWorker bgw; + decoding_worker = palloc0_object(DecodingWorker); + /* Setup shared memory. */ size = BUFFERALIGN(offsetof(DecodingWorkerShared, error_queue)) + BUFFERALIGN(REPACK_ERROR_QUEUE_SIZE); seg = dsm_create(size, 0); + decoding_worker->seg = seg; + shared = (DecodingWorkerShared *) dsm_segment_address(seg); shared->initialized = false; shared->lsn_upto = InvalidXLogRecPtr; @@ -3454,14 +3458,12 @@ start_repack_decoding_worker(Oid relid) bgw.bgw_main_arg = UInt32GetDatum(dsm_segment_handle(seg)); bgw.bgw_notify_pid = MyProcPid; - decoding_worker = palloc0_object(DecodingWorker); if (!RegisterDynamicBackgroundWorker(&bgw, &decoding_worker->handle)) ereport(ERROR, errcode(ERRCODE_CONFIGURATION_LIMIT_EXCEEDED), errmsg("out of background worker slots"), errhint("You might need to increase \"%s\".", "max_worker_processes")); - decoding_worker->seg = seg; decoding_worker->error_mqh = mqh; /* @@ -3487,17 +3489,6 @@ start_repack_decoding_worker(Oid relid) ConditionVariableCancelSleep(); } -/* - * PG_ENSURE_ERROR_CLEANUP callback to stop the decoding worker. - * This ensures the worker is terminated on both ERROR and FATAL exits, - * unlike PG_FINALLY which only handles ERROR. - */ -static void -repack_decoding_worker_cleanup_cb(int code, Datum arg) -{ - stop_repack_decoding_worker(); -} - /* * Stop the decoding worker and cleanup the related resources. * @@ -3508,39 +3499,43 @@ static void stop_repack_decoding_worker(void) { BgwHandleStatus status; + dsm_segment *dsmseg; - /* Haven't reached the worker startup? */ + /* Nothing to do if no worker was set up. */ if (decoding_worker == NULL) return; - /* Could not register the worker? */ - if (decoding_worker->handle == NULL) - return; + /* Terminate the worker process, if one is running. */ + if (decoding_worker->handle != NULL) + { + TerminateBackgroundWorker(decoding_worker->handle); + /* The worker should really exit before the REPACK command does. */ + HOLD_INTERRUPTS(); + status = WaitForBackgroundWorkerShutdown(decoding_worker->handle); + RESUME_INTERRUPTS(); - TerminateBackgroundWorker(decoding_worker->handle); - /* The worker should really exit before the REPACK command does. */ - HOLD_INTERRUPTS(); - status = WaitForBackgroundWorkerShutdown(decoding_worker->handle); - RESUME_INTERRUPTS(); - - if (status == BGWH_POSTMASTER_DIED) - ereport(FATAL, - errcode(ERRCODE_ADMIN_SHUTDOWN), - errmsg("postmaster exited during REPACK command")); - - shm_mq_detach(decoding_worker->error_mqh); + if (status == BGWH_POSTMASTER_DIED) + ereport(FATAL, + errcode(ERRCODE_ADMIN_SHUTDOWN), + errmsg("postmaster exited during REPACK command")); + } /* - * If we could not cancel the current sleep due to ERROR, do that before - * we detach from the shared memory the condition variable is located in. - * If we did not, the bgworker ERROR handling code would try and fail - * badly. + * Now detach from our shared memory segment. In error cases there might + * still be messages from the worker in the queue, which ProcessInterrupts + * would try to read; this is pointless (and causes an assertion failure), + * so set the global pointer to NULL to have ProcessRepackMessages ignore + * them. */ - ConditionVariableCancelSleep(); - - dsm_detach(decoding_worker->seg); + dsmseg = decoding_worker->seg; pfree(decoding_worker); decoding_worker = NULL; + + /* We must also cancel the current sleep, if one is still set up */ + ConditionVariableCancelSleep(); + + if (dsmseg != NULL) + dsm_detach(dsmseg); } /* stop_repack_decoding_worker, wrapped as a before_shmem_exit callback */ -- 2.47.3 --b42ct6adeyjj4cbm-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 41+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 41+ messages in thread
end of thread, other threads:[~2026-07-01 13:18 UTC | newest] Thread overview: 41+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-05-18 17:13 [PATCH] Restructure repack worker teardown Álvaro Herrera <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox