From e9e976d3311ee0ef3f274edff95970dc6fbfdcdf Mon Sep 17 00:00:00 2001 From: Mehmet Yilmaz Date: Mon, 23 Jun 2025 16:35:37 +0000 Subject: [PATCH 1/3] Refactor UpdateStripeMetadataRow to handle locking modes based on PostgreSQL version --- src/backend/columnar/columnar_metadata.c | 85 +++++++++++------------- 1 file changed, 39 insertions(+), 46 deletions(-) diff --git a/src/backend/columnar/columnar_metadata.c b/src/backend/columnar/columnar_metadata.c index 301596b26..f0e389038 100644 --- a/src/backend/columnar/columnar_metadata.c +++ b/src/backend/columnar/columnar_metadata.c @@ -1391,7 +1391,15 @@ UpdateStripeMetadataRow(uint64 storageId, uint64 stripeId, bool *update, Oid columnarStripesOid = ColumnarStripeRelationId(); - Relation columnarStripes = table_open(columnarStripesOid, AccessShareLock); +#if PG_VERSION_NUM >= 180000 + /* CatalogTupleUpdate performs a normal heap UPDATE → RowExclusiveLock */ + const LOCKMODE openLockMode = RowExclusiveLock; +#else + /* In‑place update never changed tuple length → AccessShareLock was enough */ + const LOCKMODE openLockMode = AccessShareLock; +#endif + + Relation columnarStripes = table_open(columnarStripesOid, openLockMode); Oid indexId = ColumnarStripePKeyIndexRelationId(); bool indexOk = OidIsValid(indexId); @@ -1405,58 +1413,43 @@ UpdateStripeMetadataRow(uint64 storageId, uint64 stripeId, bool *update, loggedSlowMetadataAccessWarning = true; } - HeapTuple oldTuple = systable_getnext(scanDescriptor); - if (!HeapTupleIsValid(oldTuple)) - { - ereport(ERROR, (errmsg("attempted to modify an unexpected stripe, " - "columnar storage with id=" UINT64_FORMAT - " does not have stripe with id=" UINT64_FORMAT, - storageId, stripeId))); - } + HeapTuple oldTuple = systable_getnext(scanDescriptor); + if (!HeapTupleIsValid(oldTuple)) + ereport(ERROR, + (errmsg("attempted to modify an unexpected stripe, " + "columnar storage with id=" UINT64_FORMAT + " does not have stripe with id=" UINT64_FORMAT, + storageId, stripeId))); + /* ---------------- construct the new tuple ---------------- */ + bool newNulls[Natts_columnar_stripe] = {false}; + TupleDesc tupleDescriptor = RelationGetDescr(columnarStripes); + HeapTuple modifiedTuple = heap_modify_tuple(oldTuple, + tupleDescriptor, + newValues, + newNulls, + update); -/* - * heap_modify_tuple + heap_inplace_update only exist on PG < 18; - * on PG18 the in-place helper was removed upstream, so we skip the whole block. - */ -#if PG_VERSION_NUM < PG_VERSION_18 - - /* - * heap_inplace_update already doesn't allow changing size of the original - * tuple, so we don't allow setting any Datum's to NULL values. - */ - bool newNulls[Natts_columnar_stripe] = { false }; - TupleDesc tupleDescriptor = RelationGetDescr(columnarStripes); - HeapTuple modifiedTuple = heap_modify_tuple(oldTuple, - tupleDescriptor, - newValues, - newNulls, - update); - - heap_inplace_update(columnarStripes, modifiedTuple); +#if PG_VERSION_NUM < 180000 + /* Fast path: true in‑place update (same physical tuple) */ + heap_inplace_update(columnarStripes, modifiedTuple); + HeapTuple newTuple = oldTuple; /* contents overwritten in place */ +#else + /* Regular catalog UPDATE keeps indexes in sync */ + CatalogTupleUpdate(columnarStripes, &oldTuple->t_self, modifiedTuple); + HeapTuple newTuple = modifiedTuple; /* freshly written tuple */ #endif + CommandCounterIncrement(); - /* - * Existing tuple now contains modifications, because we used - * heap_inplace_update(). - */ - HeapTuple newTuple = oldTuple; + /* Build StripeMetadata from the up‑to‑date tuple */ + StripeMetadata *modifiedStripeMetadata = + BuildStripeMetadata(columnarStripes, newTuple); - /* - * Must not pass modifiedTuple, because BuildStripeMetadata expects a real - * heap tuple with MVCC fields. - */ - StripeMetadata *modifiedStripeMetadata = BuildStripeMetadata(columnarStripes, - newTuple); + systable_endscan(scanDescriptor); + table_close(columnarStripes, openLockMode); - CommandCounterIncrement(); - - systable_endscan(scanDescriptor); - table_close(columnarStripes, AccessShareLock); - - /* return StripeMetadata object built from modified tuple */ - return modifiedStripeMetadata; + return modifiedStripeMetadata; } From fc93466516f82d974a35643af2c3540bec921368 Mon Sep 17 00:00:00 2001 From: Mehmet Yilmaz Date: Mon, 23 Jun 2025 17:08:35 +0000 Subject: [PATCH 2/3] Refactor UpdateStripeMetadataRow for improved readability and consistency --- src/backend/columnar/columnar_metadata.c | 72 +++++++++++++----------- 1 file changed, 39 insertions(+), 33 deletions(-) diff --git a/src/backend/columnar/columnar_metadata.c b/src/backend/columnar/columnar_metadata.c index f0e389038..9ce2065ec 100644 --- a/src/backend/columnar/columnar_metadata.c +++ b/src/backend/columnar/columnar_metadata.c @@ -1392,14 +1392,16 @@ UpdateStripeMetadataRow(uint64 storageId, uint64 stripeId, bool *update, Oid columnarStripesOid = ColumnarStripeRelationId(); #if PG_VERSION_NUM >= 180000 - /* CatalogTupleUpdate performs a normal heap UPDATE → RowExclusiveLock */ - const LOCKMODE openLockMode = RowExclusiveLock; + + /* CatalogTupleUpdate performs a normal heap UPDATE → RowExclusiveLock */ + const LOCKMODE openLockMode = RowExclusiveLock; #else - /* In‑place update never changed tuple length → AccessShareLock was enough */ - const LOCKMODE openLockMode = AccessShareLock; + + /* In‑place update never changed tuple length → AccessShareLock was enough */ + const LOCKMODE openLockMode = AccessShareLock; #endif - Relation columnarStripes = table_open(columnarStripesOid, openLockMode); + Relation columnarStripes = table_open(columnarStripesOid, openLockMode); Oid indexId = ColumnarStripePKeyIndexRelationId(); bool indexOk = OidIsValid(indexId); @@ -1413,43 +1415,47 @@ UpdateStripeMetadataRow(uint64 storageId, uint64 stripeId, bool *update, loggedSlowMetadataAccessWarning = true; } - HeapTuple oldTuple = systable_getnext(scanDescriptor); - if (!HeapTupleIsValid(oldTuple)) - ereport(ERROR, - (errmsg("attempted to modify an unexpected stripe, " - "columnar storage with id=" UINT64_FORMAT - " does not have stripe with id=" UINT64_FORMAT, - storageId, stripeId))); + HeapTuple oldTuple = systable_getnext(scanDescriptor); + if (!HeapTupleIsValid(oldTuple)) + { + ereport(ERROR, + (errmsg("attempted to modify an unexpected stripe, " + "columnar storage with id=" UINT64_FORMAT + " does not have stripe with id=" UINT64_FORMAT, + storageId, stripeId))); + } - /* ---------------- construct the new tuple ---------------- */ - bool newNulls[Natts_columnar_stripe] = {false}; - TupleDesc tupleDescriptor = RelationGetDescr(columnarStripes); - HeapTuple modifiedTuple = heap_modify_tuple(oldTuple, - tupleDescriptor, - newValues, - newNulls, - update); + /* ---------------- construct the new tuple ---------------- */ + bool newNulls[Natts_columnar_stripe] = { false }; + TupleDesc tupleDescriptor = RelationGetDescr(columnarStripes); + HeapTuple modifiedTuple = heap_modify_tuple(oldTuple, + tupleDescriptor, + newValues, + newNulls, + update); #if PG_VERSION_NUM < 180000 - /* Fast path: true in‑place update (same physical tuple) */ - heap_inplace_update(columnarStripes, modifiedTuple); - HeapTuple newTuple = oldTuple; /* contents overwritten in place */ + + /* Fast path: true in‑place update (same physical tuple) */ + heap_inplace_update(columnarStripes, modifiedTuple); + HeapTuple newTuple = oldTuple; /* contents overwritten in place */ #else - /* Regular catalog UPDATE keeps indexes in sync */ - CatalogTupleUpdate(columnarStripes, &oldTuple->t_self, modifiedTuple); - HeapTuple newTuple = modifiedTuple; /* freshly written tuple */ + + /* Regular catalog UPDATE keeps indexes in sync */ + CatalogTupleUpdate(columnarStripes, &oldTuple->t_self, modifiedTuple); + HeapTuple newTuple = modifiedTuple; /* freshly written tuple */ #endif - CommandCounterIncrement(); + CommandCounterIncrement(); - /* Build StripeMetadata from the up‑to‑date tuple */ - StripeMetadata *modifiedStripeMetadata = - BuildStripeMetadata(columnarStripes, newTuple); + /* Build StripeMetadata from the up‑to‑date tuple */ + StripeMetadata *modifiedStripeMetadata = + BuildStripeMetadata(columnarStripes, newTuple); - systable_endscan(scanDescriptor); - table_close(columnarStripes, openLockMode); + systable_endscan(scanDescriptor); + table_close(columnarStripes, openLockMode); - return modifiedStripeMetadata; + return modifiedStripeMetadata; } From 096a510dfe970d1d0a49b6caf802c3e64c1e7c08 Mon Sep 17 00:00:00 2001 From: Mehmet Yilmaz Date: Mon, 23 Jun 2025 17:20:38 +0000 Subject: [PATCH 3/3] Refactor UpdateStripeMetadataRow for improved error handling and code clarity --- src/backend/columnar/columnar_metadata.c | 32 ++++++++++++++++-------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/src/backend/columnar/columnar_metadata.c b/src/backend/columnar/columnar_metadata.c index 9ce2065ec..8c252a906 100644 --- a/src/backend/columnar/columnar_metadata.c +++ b/src/backend/columnar/columnar_metadata.c @@ -1418,14 +1418,12 @@ UpdateStripeMetadataRow(uint64 storageId, uint64 stripeId, bool *update, HeapTuple oldTuple = systable_getnext(scanDescriptor); if (!HeapTupleIsValid(oldTuple)) { - ereport(ERROR, - (errmsg("attempted to modify an unexpected stripe, " - "columnar storage with id=" UINT64_FORMAT - " does not have stripe with id=" UINT64_FORMAT, - storageId, stripeId))); + ereport(ERROR, (errmsg("attempted to modify an unexpected stripe, " + "columnar storage with id=" UINT64_FORMAT + " does not have stripe with id=" UINT64_FORMAT, + storageId, stripeId))); } - /* ---------------- construct the new tuple ---------------- */ bool newNulls[Natts_columnar_stripe] = { false }; TupleDesc tupleDescriptor = RelationGetDescr(columnarStripes); HeapTuple modifiedTuple = heap_modify_tuple(oldTuple, @@ -1434,27 +1432,39 @@ UpdateStripeMetadataRow(uint64 storageId, uint64 stripeId, bool *update, newNulls, update); -#if PG_VERSION_NUM < 180000 +#if PG_VERSION_NUM < PG_VERSION_18 - /* Fast path: true in‑place update (same physical tuple) */ + /* + * heap_inplace_update already doesn't allow changing size of the original + * tuple, so we don't allow setting any Datum's to NULL values. + */ heap_inplace_update(columnarStripes, modifiedTuple); - HeapTuple newTuple = oldTuple; /* contents overwritten in place */ + + /* + * Existing tuple now contains modifications, because we used + * heap_inplace_update(). + */ + HeapTuple newTuple = oldTuple; #else /* Regular catalog UPDATE keeps indexes in sync */ CatalogTupleUpdate(columnarStripes, &oldTuple->t_self, modifiedTuple); - HeapTuple newTuple = modifiedTuple; /* freshly written tuple */ + HeapTuple newTuple = modifiedTuple; #endif CommandCounterIncrement(); - /* Build StripeMetadata from the up‑to‑date tuple */ + /* + * Must not pass modifiedTuple, because BuildStripeMetadata expects a real + * heap tuple with MVCC fields. + */ StripeMetadata *modifiedStripeMetadata = BuildStripeMetadata(columnarStripes, newTuple); systable_endscan(scanDescriptor); table_close(columnarStripes, openLockMode); + /* return StripeMetadata object built from modified tuple */ return modifiedStripeMetadata; }