Keep temp reloid for columnar cases #8309

manaldush · 2025-10-31T11:14:49Z

PG18 and PG latest minors ignore temporary relations in RelidByRelfilenumber (RelidByRelfilenode in PG15)
Relevant PG commit:
postgres/postgres@86831952

Here we are keeping temp reloids instead of getting it with RelidByRelfilenumber, for example, in some cases, we can directly get reloid from relations, in other cases we keep it in some structures.

Note: there is still an outstanding issue with columnar temp tables in concurrent sessions, that will be fixed in PR #8252

Alternative PR to #8303
Comparing with #8303, this approach avoids copy-paste of PG vanilla codebase

naisila

Currently evaluating this solution.

@manaldush UPDATE: after taking advice from my colleague @colm-mchugh as well, we decided this approach is better compared to #8303 for two reasons:

it avoids an extra scan of pg_class for temp tables.
It doesn't revert behavior that PG identified as broken, like #8303 does.

We also need comments for ColumnarRelationId and RelationPrecomputeOid_compat. Thanks for the PR.

naisila · 2025-11-05T09:30:37Z

src/include/pg_version_compat.h

 #define RelationPhysicalIdentifierBackend_compat(a) (a->smgr_rnode.node)
 typedef RelFileNode RelFileLocator;
 typedef Oid RelFileNumber;
-#define RelidByRelfilenumber(a, b) RelidByRelfilenode(a, b)


Should keep this as it's needed for PG15 support.

naisila · 2025-11-05T09:31:25Z

src/include/pg_version_compat.h


 #define RelationPhysicalIdentifier_compat(a) ((a)->rd_locator)
 #define RelationTablespace_compat(a) (a.spcOid)
+#define RelationPrecomputeOid_compat(a) (RelationUsesLocalBuffers(a) ? RelationGetRelid( \


This should be defined in columnar.h because all PG versions need it (we use pg_version_compat.h file for functions that need compatibility changes between different PG versions)

Fixed and renamed according with #8309 (comment)

naisila · 2025-11-05T09:32:29Z

src/backend/columnar/columnar_writer.c

 	TupleDesc tupleDescriptor;
 	FmgrInfo **comparisonFunctionArray;
 	RelFileLocator relfilelocator;
+	Oid temp_relid;          /* We can't rely on RelidByRelfilenumber for temp tables anymore.*/


Suggested change

Oid temp_relid; /* We can't rely on RelidByRelfilenumber for temp tables anymore.*/

Oid temp_relid; /* We can't rely on RelidByRelfilenumber for temp tables since PG18 */

I see change in RelidByRelfilenumber in all version since PG13(be aware till PG16 RelidByRelfilenumber = RelidByRelfilenumber), so fixed with since PG13. If you have some doubts about this change, please notify me, I will fix it

naisila · 2025-11-05T11:39:35Z

src/include/pg_version_compat.h


 #define RelationPhysicalIdentifier_compat(a) ((a)->rd_locator)
 #define RelationTablespace_compat(a) (a.spcOid)
+#define RelationPrecomputeOid_compat(a) (RelationUsesLocalBuffers(a) ? RelationGetRelid( \


Suggested change

#define RelationPrecomputeOid_compat(a) (RelationUsesLocalBuffers(a) ? RelationGetRelid( \

#define RelationPrecomputeOid(a) (RelationUsesLocalBuffers(a) ? RelationGetRelid( \

Fixed and moved to columnar.h according to #8309 (comment)

codecov · 2025-11-06T12:58:32Z

Codecov Report

❌ Patch coverage is 0% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.18%. Comparing base (188c182) to head (163bae4).
⚠️ Report is 20 commits behind head on m3hm3t/pg18_beta_confs.

❌ Your project check has failed because the head coverage (62.18%) is below the target coverage (87.50%). You can increase the head coverage or adjust the target coverage.

❗ There is a different number of reports uploaded between BASE (188c182) and HEAD (163bae4). Click for more details.

HEAD has 75 uploads less than BASE

Flag BASE (188c182) HEAD (163bae4)

17_regress_check-pytest 1 0

15_regress_check-columnar-isolation 1 0

16_regress_check-follower-cluster 1 0

15_citus_upgrade 1 0

16_regress_check-columnar-isolation 1 0

17_regress_check-follower-cluster 1 0

15_17_upgrade 1 0

15_regress_check-follower-cluster 1 0

17_regress_check-columnar-isolation 1 0

17_regress_check-add-backup-node 1 0

15_regress_check-enterprise-isolation-logicalrep-2 1 0

17_regress_check-enterprise-isolation-logicalrep-3 1 0

16_regress_check-add-backup-node 1 0

15_16_upgrade 1 0

16_regress_check-split 1 0

17_regress_check-query-generator 1 0

15_regress_check-add-backup-node 1 0

16_17_upgrade 1 0

16_regress_check-enterprise-isolation-logicalrep-3 1 0

15_regress_check-enterprise-isolation-logicalrep-3 1 0

17_regress_check-enterprise-isolation-logicalrep-2 1 0

16_regress_check-enterprise-isolation-logicalrep-2 1 0

16_regress_check-query-generator 1 0

15_regress_check-query-generator 1 0

15_regress_check-columnar 1 0

15_regress_check-split 1 0

17_regress_check-enterprise-failure 1 0

16_regress_check-columnar 1 0

16_regress_check-enterprise-failure 1 0

17_regress_check-columnar 1 0

15_regress_check-vanilla 1 0

15_regress_check-failure 1 0

16_regress_check-failure 1 0

15_regress_check-enterprise-failure 1 0

17_regress_check-enterprise 1 0

15_regress_check-enterprise 1 0

17_regress_check-split 1 0

16_regress_check-vanilla 1 0

16_regress_check-enterprise 1 0

17_regress_check-vanilla 1 0

15_regress_check-enterprise-isolation 1 0

16_regress_check-enterprise-isolation 1 0

17_regress_check-enterprise-isolation 1 0

17_regress_check-enterprise-isolation-logicalrep-1 1 0

16_regress_check-enterprise-isolation-logicalrep-1 1 0

17_regress_check-failure 1 0

17_regress_check-multi-mx 1 0

16_regress_check-multi-mx 1 0

15_regress_check-enterprise-isolation-logicalrep-1 1 0

15_regress_check-multi-mx 1 0

17_cdc_installcheck 1 0

16_citus_upgrade 1 0

15_regress_check-pytest 1 0

16_regress_check-pytest 1 0

15_cdc_installcheck 1 0

16_regress_check-multi 1 0

15_arbitrary_configs_3 1 0

16_arbitrary_configs_3 1 0

16_arbitrary_configs_5 1 0

17_arbitrary_configs_2 1 0

17_arbitrary_configs_5 1 0

15_regress_check-multi 1 0

17_regress_check-multi 1 0

15_regress_check-multi-1 1 0

17_regress_check-multi-1 1 0

16_arbitrary_configs_0 1 0

16_regress_check-multi-1 1 0

16_arbitrary_configs_2 1 0

17_arbitrary_configs_3 1 0

17_arbitrary_configs_4 1 0

15_arbitrary_configs_5 1 0

15_arbitrary_configs_1 1 0

17_arbitrary_configs_1 1 0

16_cdc_installcheck 1 0

16_arbitrary_configs_1 1 0

Additional details and impacted files

@@                     Coverage Diff                     @@
##           m3hm3t/pg18_beta_confs    #8309       +/-   ##
===========================================================
- Coverage                   88.96%   62.18%   -26.78%     
===========================================================
  Files                         287      287               
  Lines                       63134    62956      -178     
  Branches                     7941     7895       -46     
===========================================================
- Hits                        56165    39148    -17017     
- Misses                       4660    20826    +16166     
- Partials                     2309     2982      +673

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

naisila · 2025-11-06T13:04:04Z

src/backend/columnar/columnar_metadata.c

 * to get correct relid value.
 *
- * Now it is basically used for temp rels, because since PGXX RelidByRelfilenumber
+ * Now it is basically used for temp rels, because since PG13 RelidByRelfilenumber


Might be a bit misleading since it was a backport to PG13. Lets say sth like "since PG18 (it was backpatched through PG13)"
Same comment for columnar.h file

naisila

Also citusdata:m3hm3t/pg18_beta_confs has been force pushed so could you update the PR accordingly?

EDIT: @manaldush in fact we use citusdata:m3hm3t/pg18_beta_confs for sanity checks only, and we merge directly to main branch, so it would be great if you can update the base branch to citusdata:main and include ONLY your commits.

After PG vanilla commit ab874faaa1f, RelidByRelfilenumber logic was changed, now for temp objects its return O oid. For such cases we can use alternative approach with keeping temp relations oids directly.

Fix PR comments.

manaldush · 2025-11-06T14:17:18Z

Also citusdata:m3hm3t/pg18_beta_confs has been force pushed so could you update the PR accordingly?

EDIT: @manaldush in fact we use citusdata:m3hm3t/pg18_beta_confs for sanity checks only, and we merge directly to main branch, so it would be great if you can update the base branch to citusdata:main and include ONLY your commits.

done, now branch m3hm3t/pg18_beta_confs_8235 branched from main.

naisila

LGTM, thanks

Backporting citusdata#8235 PG18 and PG latest minors ignore temporary relations in `RelidByRelfilenumber` (`RelidByRelfilenode` in PG15) Relevant PG commit: postgres/postgres@86831952 Here we are keeping temp reloids instead of getting it with RelidByRelfilenumber, for example, in some cases, we can directly get reloid from relations, in other cases we keep it in some structures. Note: there is still an outstanding issue with columnar temp tables in concurrent sessions, that will be fixed in PR citusdata#8252

Backporting citusdata#8235 PG18 and PG latest minors ignore temporary relations in `RelidByRelfilenumber` (`RelidByRelfilenode` in PG15) Relevant PG commit: postgres/postgres@86831952 Here we are keeping temp reloids instead of getting it with RelidByRelfilenumber, for example, in some cases, we can directly get reloid from relations, in other cases we keep it in some structures. Note: there is still an outstanding issue with columnar temp tables in concurrent sessions, that will be fixed in PR citusdata#8252 (cherry picked from commit daa69be)

m3hm3t force-pushed the m3hm3t/pg18_beta_confs branch 4 times, most recently from 5f5da94 to 15ecb4b Compare November 5, 2025 09:11

naisila mentioned this pull request Nov 5, 2025

Use RelidByRelfilenumberWithTempTables instead of RelidByRelfilenumber #8303

Closed

naisila reviewed Nov 5, 2025

View reviewed changes

naisila added the pg18_support label Nov 5, 2025

naisila linked an issue Nov 5, 2025 that may be closed by this pull request

PG18 - columnar temp tables cannot be accessed "could not open relation with oid 0" #8235

Closed

naisila reviewed Nov 5, 2025

View reviewed changes

m3hm3t force-pushed the m3hm3t/pg18_beta_confs branch from 15ecb4b to 5565920 Compare November 5, 2025 18:45

naisila reviewed Nov 6, 2025

View reviewed changes

manaldush added 4 commits November 6, 2025 16:54

Columnar temp tables cannot be accessed(citusdata#8235).

d357820

After PG vanilla commit ab874faaa1f, RelidByRelfilenumber logic was changed, now for temp objects its return O oid. For such cases we can use alternative approach with keeping temp relations oids directly.

Columnar temp tables cannot be accessed(citusdata#8235).

e7f7677

Fix PR comments.

Columnar temp tables cannot be accessed(citusdata#8235).

2863088

Fix PR comments.

Columnar temp tables cannot be accessed(citusdata#8235).

cbf097f

Fix PR comments.

manaldush force-pushed the m3hm3t/pg18_beta_confs_8235 branch from 28af426 to cbf097f Compare November 6, 2025 14:15

naisila changed the base branch from m3hm3t/pg18_beta_confs to main November 6, 2025 20:13

naisila approved these changes Nov 6, 2025

View reviewed changes

naisila merged commit daa69be into citusdata:main Nov 6, 2025
145 of 158 checks passed

	Oid temp_relid; /* We can't rely on RelidByRelfilenumber for temp tables anymore.*/
	Oid temp_relid; /* We can't rely on RelidByRelfilenumber for temp tables since PG18 */

	#define RelationPrecomputeOid_compat(a) (RelationUsesLocalBuffers(a) ? RelationGetRelid( \
	#define RelationPrecomputeOid(a) (RelationUsesLocalBuffers(a) ? RelationGetRelid( \

Keep temp reloid for columnar cases #8309

Keep temp reloid for columnar cases #8309

Uh oh!

Conversation

manaldush commented Oct 31, 2025 • edited by naisila Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

naisila left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

naisila Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naisila left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manaldush commented Nov 6, 2025

Uh oh!

naisila left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

manaldush commented Oct 31, 2025 •

edited by naisila

Loading

naisila left a comment •

edited

Loading

codecov bot commented Nov 6, 2025 •

edited

Loading

naisila Nov 6, 2025 •

edited

Loading

naisila left a comment •

edited

Loading