Oracle rowid feature #749
Oracle rowid feature #749
Conversation
WalkthroughThe changes integrate comprehensive support for a new row identifier (ROWID) across the system. Modifications span heap tuple management, tuple descriptor creation, catalog updates, executor and parser enhancements, and adjustments in SQL scripts, Makefiles, configuration parameters, and testing frameworks. New flags, parameters, macros, and commands are introduced to consistently manage the ROWID feature within tuple formation, query execution, DDL commands, and Oracle compatibility functions. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant P as Parser
participant Q as Planner
participant E as Executor
participant H as Heap Access
participant Cat as Catalog
C->>P: Submit SQL with ROWID option
P->>Q: Translate query with rowid flag
Q->>E: Build plan using CreateTupleDesc(tdhasrowid)
E->>E: Initialize Junk Filter (hasrowid flag)
E->>H: Insert/Update tuple\n(HeapTupleSetRowId called)
H->>Cat: Update relation metadata with rowid
E->>C: Return tuple including ROWID
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (16)
src/include/utils/rel.h (1)
114-114: New field added to RelationData structure for ROWID feature.The addition of
rd_rowdSeqidfield is appropriately placed after the relation's object ID and is of the correct type (Oid) for storing a sequence identifier related to the ROWID feature.Consider adding a comment to explain the purpose of this field, particularly as it relates to the ROWID feature being implemented across the system:
Oid rd_id; /* relation's object id */ - Oid rd_rowdSeqid; + Oid rd_rowdSeqid; /* sequence id for ROWID generation */ LockInfoData rd_lockInfo; /* lock mgr's info for locking relation */src/backend/executor/nodeForeignscan.c (1)
192-192: Updated tuple descriptor creation to support ROWIDThe function call has been modified to add a boolean parameter (
false) which likely controls whether the ROWID attribute should be included in the tuple descriptor. This is a necessary change to support the Oracle rowid feature.However, consider adding a comment explaining what the boolean parameter means for better code maintainability.
- scan_tupdesc = ExecTypeFromTL(node->fdw_scan_tlist, false); + /* false = don't include ROWID in the tuple descriptor */ + scan_tupdesc = ExecTypeFromTL(node->fdw_scan_tlist, false);src/bin/psql/describe.c (1)
32-32: Remove duplicate include statement.The header file "oracle_fe_utils/ora_string_utils.h" is already included on line 29, making this include redundant.
-#include "oracle_fe_utils/ora_string_utils.h"src/backend/access/common/tupdesc.c (1)
118-120: ROWID handling in attribute count calculationThis logic adjusts the attribute count when
tdhasrowidis false andis_sysattris true, which seems to handle special cases for system attributes.Consider adding a comment explaining why
nattsis decremented in this specific condition to improve code clarity.if (!tdhasrowid && is_sysattr) + /* Decrement natts to exclude the RowId system attribute from the count */ natts = natts - 1;src/backend/catalog/index.c (1)
345-359: Proper handling added for Oracle ROWID attribute in index creation.The code now correctly handles the special case when
atnumequalsRowIdAttributeNumberby retrieving the system attribute definition directly. For regular attributes, the safety checks are maintained to prevent out-of-bounds access to the tuple descriptor.It might be helpful to add a brief comment explaining why
RowIdAttributeNumberneeds special handling here, especially since this is part of a new Oracle compatibility feature.contrib/ivorysql_ora/preload_ora_misc.sql (1)
14-15: Confirm cast behavior betweenrowidandurowid.
The implicit casts allow seamless conversion in Oracle-like scenarios. If code relies on strict type checking in some places, an implicit cast might introduce silent conversions. Ensure that your queries and function calls anticipate these conversions.src/backend/executor/nodeAgg.c (2)
1673-1673: Update ExecTypeFromTL Call in find_hash_columnsThe call to
ExecTypeFromTL(hashTlist)has been updated toExecTypeFromTL(hashTlist, false). This change reflects the new function signature that requires an extra boolean parameter (presumably to indicate whether a ROWID should be present). Please verify that passingfalsehere is semantically correct for the grouping columns context in a non-ROWID scenario.
4195-4195: Update ExecTypeFromTL Call in build_pertrans_for_aggrefSimilarly, the call originally used as
pertrans->sortdesc = ExecTypeFromTL(aggref->args);has now been revised to include the extra parameter:
pertrans->sortdesc = ExecTypeFromTL(aggref->args, false);This change aligns with the updated function signature for supporting Oracle ROWID compatibility. Please confirm that the hardcoded
falsecorrectly reflects the desired behavior for tuple descriptor generation in this transition-phase context.src/pl/plisql/src/pl_exec.c (1)
7469-7472: Consider error handling for memory allocation failuresMemory allocation with
pallocshould be checked for success, especially when allocating potentially large arrays. Consider adding error handling or usingpalloc0which will at least set the memory to zeros.-r_values = (Datum *) palloc(rowidtupdesc->natts * sizeof(Datum)); -r_nulls = (bool *) palloc(rowidtupdesc->natts * sizeof(bool)); +r_values = (Datum *) palloc0(rowidtupdesc->natts * sizeof(Datum)); +r_nulls = (bool *) palloc0(rowidtupdesc->natts * sizeof(bool)); + +if (r_values == NULL || r_nulls == NULL) + ereport(ERROR, + (errcode(ERRCODE_OUT_OF_MEMORY), + errmsg("out of memory")));src/include/access/htup_details.h (1)
69-71: Update comment to include ROWID referenceThe comment mentions HEAP_HASOID_OLD but should be updated to also mention HEAP_HASROWID since this flag is replacing the old OID functionality.
* alignment padding (as needed to make user data MAXALIGN'd) -* object ID (if HEAP_HASOID_OLD is set in t_infomask, not created -* anymore) +* object ID (if HEAP_HASOID_OLD is set in t_infomask, not created +* anymore) or row ID (if HEAP_HASROWID is set in t_infomask) * user data fieldssrc/backend/access/common/heaptuple.c (1)
762-790: Creating a composite Datum for the rowid is correct but might incur overhead.
Allocating a separate tuple purely for returning the rowid is valid, though it may be somewhat heavy for frequently accessed rows. A more lightweight representation (e.g., returning a domain or a built-in type) might improve performance.src/backend/commands/tablecmds.c (2)
702-704: Consider renamingloclHasRowidfor clarity.Renaming it to
localHasRowidmight better reflect the variable’s purpose.
6222-6224: Naming consistency concern withrd_rowdSeqid.Consider renaming
rd_rowdSeqidtord_rowidSeqidfor clarity and consistency.src/backend/parser/parse_utilcmd.c (1)
3638-3710: Significant duplication in rowid sequence creation logic.The block for
AT_AddRowidsRecursemostly duplicates the earlier rowid sequence creation steps (lines 349-414). While it correctly handles the ALTER TABLE path, consider refactoring to a shared helper function to reduce maintenance overhead and keep the logic DRY.- /* Repeated rowid-sequence creation logic here. */ + /* + * Suggestion: Extract rowid-sequence creation logic into a new helper + * function, for example: + * static void create_rowid_sequence(CreateStmtContext *cxt) { ... } + * Then call it both in the CREATE TABLE path and inside AT_AddRowidsRecurse. + */src/backend/utils/cache/relcache.c (2)
904-920: Consider more efficient string construction for sequence name.The sequence name construction uses multiple
appendStringInfocalls which could be replaced with a single call for better efficiency.This could be simplified to:
- initStringInfo(&rowid_seq); - appendStringInfo(&rowid_seq, "%s", RelationGetRelationName(relation)); - appendStringInfoChar(&rowid_seq, '_'); - appendStringInfo(&rowid_seq, "rowid"); - appendStringInfoChar(&rowid_seq, '_'); - appendStringInfo(&rowid_seq, "seq"); + initStringInfo(&rowid_seq); + appendStringInfo(&rowid_seq, "%s_rowid_seq", RelationGetRelationName(relation));
939-944: Add error handling for missing rowid sequence.The function doesn't handle the case where no sequence is found. This could lead to using an uninitialized
seqoidvalue (which we're fixing separately), but we should also log a warning when this happens.Add error handling after the scan:
while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL) { seqoid = ((Form_pg_class) GETSTRUCT(tuple))->oid; } +if (seqoid == InvalidOid) +{ + ereport(WARNING, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("could not find rowid sequence for relation \"%s\"", + RelationGetRelationName(relation)))); +} relation->rd_rowdSeqid = seqoid;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
contrib/pageinspect/expected/ivy_page.outis excluded by!**/*.outcontrib/pageinspect/expected/page.outis excluded by!**/*.outcontrib/tablefunc/expected/ivy_tablefunc.outis excluded by!**/*.outsrc/include/catalog/pg_type.datis excluded by!**/*.datsrc/oracle_test/regress/expected/alter_table.outis excluded by!**/*.outsrc/oracle_test/regress/expected/ora_rowid.outis excluded by!**/*.outsrc/oracle_test/regress/expected/type_sanity.outis excluded by!**/*.out
📒 Files selected for processing (66)
contrib/amcheck/verify_heapam.c(1 hunks)contrib/ivorysql_ora/Makefile(1 hunks)contrib/ivorysql_ora/preload_ora_misc.sql(1 hunks)contrib/ivorysql_ora/src/sysview/sysview--1.0.sql(0 hunks)contrib/pageinspect/heapfuncs.c(1 hunks)contrib/tablefunc/sql/ivy_tablefunc.sql(5 hunks)src/backend/access/common/heaptuple.c(7 hunks)src/backend/access/common/reloptions.c(1 hunks)src/backend/access/common/tupdesc.c(4 hunks)src/backend/access/heap/heapam.c(5 hunks)src/backend/access/heap/heaptoast.c(5 hunks)src/backend/bootstrap/bootparse.y(1 hunks)src/backend/bootstrap/bootstrap.c(1 hunks)src/backend/catalog/Catalog.pm(1 hunks)src/backend/catalog/aclchk.c(1 hunks)src/backend/catalog/heap.c(9 hunks)src/backend/catalog/index.c(1 hunks)src/backend/commands/createas.c(2 hunks)src/backend/commands/indexcmds.c(2 hunks)src/backend/commands/matview.c(1 hunks)src/backend/commands/sequence.c(3 hunks)src/backend/commands/tablecmds.c(26 hunks)src/backend/executor/execExpr.c(1 hunks)src/backend/executor/execJunk.c(2 hunks)src/backend/executor/execMain.c(2 hunks)src/backend/executor/execTuples.c(5 hunks)src/backend/executor/execUtils.c(2 hunks)src/backend/executor/functions.c(1 hunks)src/backend/executor/nodeAgg.c(2 hunks)src/backend/executor/nodeCustom.c(1 hunks)src/backend/executor/nodeForeignscan.c(1 hunks)src/backend/executor/nodeIndexonlyscan.c(1 hunks)src/backend/executor/nodeSubplan.c(1 hunks)src/backend/executor/spi.c(1 hunks)src/backend/oracle_parser/ora_gram.y(8 hunks)src/backend/parser/analyze.c(1 hunks)src/backend/parser/parse_clause.c(2 hunks)src/backend/parser/parse_expr.c(1 hunks)src/backend/parser/parse_relation.c(1 hunks)src/backend/parser/parse_utilcmd.c(8 hunks)src/backend/tcop/pquery.c(1 hunks)src/backend/utils/adt/orderedsetaggs.c(1 hunks)src/backend/utils/cache/plancache.c(1 hunks)src/backend/utils/cache/relcache.c(17 hunks)src/backend/utils/misc/ivy_guc.c(3 hunks)src/bin/initdb/initdb.c(4 hunks)src/bin/pg_dump/pg_dump.c(6 hunks)src/bin/pg_dump/pg_dump.h(1 hunks)src/bin/psql/describe.c(5 hunks)src/include/access/htup_details.h(2 hunks)src/include/access/sysattr.h(1 hunks)src/include/access/tupdesc.h(2 hunks)src/include/catalog/pg_class.h(1 hunks)src/include/commands/event_trigger.h(1 hunks)src/include/executor/executor.h(4 hunks)src/include/nodes/parsenodes.h(4 hunks)src/include/oracle_parser/ora_kwlist.h(1 hunks)src/include/parser/parse_clause.h(1 hunks)src/include/utils/guc.h(1 hunks)src/include/utils/rel.h(1 hunks)src/oracle_test/modules/test_ddl_deparse/test_ddl_deparse.c(1 hunks)src/oracle_test/regress/GNUmakefile(3 hunks)src/oracle_test/regress/serial_schedule(1 hunks)src/oracle_test/regress/sql/ora_rowid.sql(1 hunks)src/oracle_test/regress/sql/type_sanity.sql(1 hunks)src/pl/plisql/src/pl_exec.c(4 hunks)
💤 Files with no reviewable changes (1)
- contrib/ivorysql_ora/src/sysview/sysview--1.0.sql
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/backend/utils/misc/ivy_guc.c
[error] 213-213: syntax error
(syntaxError)
src/pl/plisql/src/pl_exec.c
[warning] 7457-7457: Uninitialized variable
(uninitvar)
src/backend/utils/cache/relcache.c
[warning] 945-945: Uninitialized variable
(uninitvar)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: oracle_pg_regression (ubuntu-latest)
- GitHub Check: pg_regression (ubuntu-latest)
- GitHub Check: contrib_regression
- GitHub Check: build (ubuntu-latest)
- GitHub Check: meson_build (ubuntu-latest)
- GitHub Check: oracle_regression (ubuntu-latest)
🔇 Additional comments (174)
src/oracle_test/regress/serial_schedule (1)
1-4: Test structure looks good for ROWID feature testing.The new serial_schedule file correctly sets up a test target for "ora_rowid" to validate the ROWID functionality. The comment suggesting alignment with parallel_schedule is helpful for maintaining consistency in test organization.
src/bin/pg_dump/pg_dump.h (1)
315-315: New field for ROWID support added appropriately.The addition of the
relhasrowidboolean field to the_tableInfostructure allows pg_dump to properly handle tables with ROWID during backup and restore operations. This is a necessary change to ensure ROWIDs are preserved when dumping the database schema.src/include/catalog/pg_class.h (1)
98-99: Good implementation of ROWID support in catalog structure.The addition of the
relhasrowidboolean field to theFormData_pg_classstructure with a clear comment and default value of false (BKI_DEFAULT(f)) is appropriate. This allows tracking which relations generate ROWIDs for their rows at the catalog level, which is essential for the feature's implementation.src/include/oracle_parser/ora_kwlist.h (1)
444-444: Keyword addition for ROWID is correctly implemented.Adding "rowid" as an unreserved keyword with BARE_LABEL status ensures the SQL parser recognizes it appropriately. The keyword is properly positioned alphabetically in the list, which maintains the expected ordering required by the parser generator.
src/backend/parser/parse_relation.c (1)
39-40: Adding necessary Oracle compatibility header filesThese new includes are required to support Oracle's rowid feature. The
utils/guc.hheader provides access to configuration parameters, whileutils/ora_compatible.hcontains Oracle compatibility functions necessary for implementing the rowid feature.src/backend/executor/nodeCustom.c (1)
81-81: Updated ExecTypeFromTL call with rowid configuration parameterThe
ExecTypeFromTLfunction has been updated to accept an additional boolean parameter, which is set tofalsehere. This parameter likely indicates whether the tuple descriptor should include a rowid column. Setting it tofalsefor custom scans means they won't include rowids by default, which is appropriate for this context.src/backend/access/common/reloptions.c (1)
1267-1269: Well-integrated skip for "rowid" optionThe code now skips processing any relation option named "rowid", which integrates the Oracle ROWID feature without interfering with the existing option processing logic. This is a clean approach to handling the special ROWID option separately from standard relation options.
src/backend/utils/adt/orderedsetaggs.c (1)
217-217: ROWID parameter added to ExecTypeFromTLThe function call to
ExecTypeFromTLhas been updated to include a second boolean parameter (false), which likely controls whether to include ROWID in the resulting tuple descriptor. This change is consistent with other similar updates across the codebase for the Oracle ROWID feature.src/backend/commands/createas.c (2)
48-48: Required header inclusion for ROWID handlingAdding the parse_clause.h header is necessary to access the
interpretRowidOptionfunction used for ROWID handling.
375-380: Clean implementation of ROWID flag settingThis code appropriately sets execution flags based on whether the ROWID option is enabled, using the new
interpretRowidOptionfunction. The condition correctly checks both the table options and whether this is a regular table (not a view).The implementation ensures that CREATE TABLE AS commands properly handle the ROWID feature based on user options.
contrib/ivorysql_ora/Makefile (2)
39-40: Added install target for Oracle compatibilityThe new
installtarget facilitates the proper installation of Oracle compatibility components.
47-50: Oracle ROWID support installationThe
install-datatarget handles the installation ofpreload_ora_misc.sql, which contains necessary definitions for Oracle compatibility features including ROWID support. This ensures that the SQL scripts needed for Oracle compatibility are properly installed into the destination directory.src/backend/bootstrap/bootparse.y (1)
173-173: Function parameter addition looks goodThe function call has been updated to accommodate the new
tdhasrowidandis_sysattrparameters ofCreateTupleDesc. Setting both parameters tofalseis appropriate for bootstrap context, ensuring system catalog tuples don't have ROWID attributes by default.src/oracle_test/modules/test_ddl_deparse/test_ddl_deparse.c (1)
331-337: Added ALTER TABLE subcommands for ROWID featuresThe new case statements for
AT_AddRowids,AT_AddRowidsRecurse, andAT_DropRowidscorrectly handle the string representations of these commands. The string values are clear and consistent with PostgreSQL's SQL syntax style.src/include/utils/guc.h (1)
311-312: New GUC variables for ROWID functionalityThese new configuration variables will control the ROWID feature behavior:
default_with_rowids- Controls whether tables are created with ROWID columns by defaultrowid_seq_cache- Controls cache size for ROWID sequence operationsThe declarations follow the proper pattern with PGDLLIMPORT, consistent with other GUC variables.
src/include/parser/parse_clause.h (1)
54-54: New function for interpreting ROWID optionsThe
interpretRowidOptionfunction declaration looks good. It will parse ROWID-related options from SQL statements, with theallowRowidparameter determining if ROWIDs are permitted in the current context.contrib/amcheck/verify_heapam.c (1)
959-962: Correctly handles ROWID in tuple header size calculationsThis change appropriately adds logic to account for the additional space required in the tuple header when the ROWID flag is set. The code increases the expected header offset by the size of an int64 when the HEAP_HASROWID flag is present, consistent with how other header flags like HEAP_HASNULL are handled.
src/include/commands/event_trigger.h (1)
38-38: LGTM: New constant for ALTER_ROWID rewrite operationsThe added constant follows the established pattern of using bitwise flags (powers of 2) for the different rewrite operation types. This will cleanly integrate with the event trigger system for ROWID-related alterations.
src/backend/executor/nodeIndexonlyscan.c (1)
546-546: Updated function call to pass ROWID flag parameterThe change adds the
falseparameter to theExecTypeFromTLcall, indicating that the tuples for index-only scans don't need ROWID handling. This is appropriate since index-only scans operate on index tuples rather than heap tuples.src/backend/catalog/Catalog.pm (1)
340-340: Fixed Oracle compatibility metadata handling conditionThis change improves the processing logic for Oracle compatibility metadata by only executing the block when we've seen the start marker but not yet seen the end marker, preventing potential issues when both markers have been processed.
src/include/access/sysattr.h (1)
27-28: New system attribute for Oracle ROWID featureThe changes appropriately add a new system attribute
RowIdAttributeNumberwith value -7 and shift theFirstLowInvalidHeapAttributeNumberfrom -7 to -8 to accommodate it. This follows the established pattern for system attributes in PostgreSQL.src/backend/parser/analyze.c (1)
2465-2476: Oracle compatibility: Blocking updates to system column 'rowid'This block correctly implements Oracle-compatible behavior by preventing direct updates to the 'rowid' system column. The error message is clear and informative, making it easier for users to understand why their query failed.
src/backend/utils/cache/plancache.c (2)
1988-1988: Added new parameter to ExecCleanTypeFromTL function call.The code now passes a
falsevalue to theExecCleanTypeFromTLfunction for the target list, which controls whether type changes are allowed during result descriptor computation. This is part of the ROWID feature implementation.
1993-1993: Added new parameter to ExecCleanTypeFromTL function call for returning lists.Similar to the target list change, this modification passes
falsetoExecCleanTypeFromTLwhen processing returning lists, ensuring consistent handling of type changes for the ROWID feature.src/oracle_test/regress/sql/type_sanity.sql (1)
423-423: Excluded 'rowid' type from attribute/type cross-checking.The query has been modified to exclude the 'rowid' type from sanity checks that verify attribute properties against their corresponding type metadata. This exclusion is necessary because the ROWID type has special handling characteristics that would otherwise trigger false positives in these tests.
src/bin/initdb/initdb.c (4)
197-197: Appropriate declaration for Oracle row identifier support.This variable will store the path to the SQL file that defines Oracle's row identifier types and related infrastructure.
2831-2832: Well-structured setup for Oracle's ROWID feature.The code properly sets up the path to the preload_ora_misc.sql file only when running in Oracle compatibility mode, following the same pattern as other file path configurations.
2863-2863: Good practice to verify file existence.This checks that the preload_ora_misc.sql file exists before attempting to use it, preventing runtime errors if the file is missing.
3201-3201: Clean integration of Oracle ROWID feature.This executes the preload_ora_misc.sql script during database initialization, which creates the necessary dual table and sys.rowid/sys.urowid types for Oracle compatibility.
src/backend/tcop/pquery.c (1)
551-551: Appropriate signature update to support ROWID functionality.The updated call to
ExecCleanTypeFromTLwith the additional boolean parameter (false) correctly implements the necessary change to support the new ROWID feature. This ensures that tuple descriptors in the portal context properly handle the presence or absence of row IDs.src/backend/executor/spi.c (1)
1159-1161: Good addition for Oracle ROWID compatibility.This change appropriately preserves the row ID when modifying a tuple through the SPI interface. It's correctly placed after copying other tuple identification information.
src/backend/catalog/aclchk.c (1)
1727-1729: Proper handling of ROWID attribute for privilege managementThe added code correctly prevents the ROWID attribute from having its privileges expanded, similar to how other special columns are handled. This is essential for the Oracle ROWID feature implementation.
src/backend/commands/matview.c (1)
413-413: Appropriate flag for materialized view refresh operations.This change ensures that materialized view refresh operations don't generate unnecessary ROWIDs by passing
EXEC_FLAG_WITHOUT_ROWIDinstead of0to theExecutorStartfunction. This is consistent with the Oracle ROWID feature integration, as materialized view refreshes don't need to track row identifiers.src/backend/executor/execJunk.c (2)
70-70: Parameter propagation for ROWID support.The
hasrowidparameter is correctly passed toExecCleanTypeFromTL, ensuring that the tuple descriptor for the cleaned tuple is created with the appropriate ROWID setting.
60-60:✅ Verification successful
API change to support ROWID functionality.
The function signature has been extended to include a new
hasrowidparameter, which will determine whether the resulting tuple descriptor should include ROWID information. This change is part of the Oracle ROWID feature integration.Ensure all callers of this function have been updated to include the new parameter:
🏁 Script executed:
#!/bin/bash # Find all callers of ExecInitJunkFilter to verify they've been updated rg "ExecInitJunkFilter\s*\(" --type cLength of output: 543
API change for Oracle ROWID support verified
The function signature of
ExecInitJunkFilterinsrc/backend/executor/execJunk.chas been updated by adding the newbool hasrowidparameter. This change allows the executor to correctly determine whether ROWID information should be included. Additionally, the caller at line 70 now passes this parameter toExecCleanTypeFromTL.Verification of all callers has confirmed that the update is applied consistently throughout the codebase:
src/include/executor/executor.hsrc/backend/executor/execExpr.csrc/backend/executor/functions.csrc/backend/executor/execMain.cPlease ensure that future modifications consider the updated API signature.
src/backend/executor/execUtils.c (2)
589-589: New variable for ROWID support.Added a new boolean variable
hasrowidto track the ROWID setting required by the execution context. This supports the Oracle ROWID feature integration.
634-640: Enhanced descriptor matching to verify ROWID compatibility.This new condition checks if the execution context forces a specific ROWID setting (using
ExecContextForcesRowId) and verifies that it matches the tuple descriptor'stdhasrowidattribute. If there's a mismatch, the function returns false, indicating the target list doesn't match the tuple descriptor. This ensures consistent ROWID behavior during query execution.src/backend/executor/execExpr.c (1)
3059-3061: Clean enhancement to pass row ID information to junk filterThe change properly passes the row ID presence information (
tdhasrowid) from the subplan's result type toExecInitJunkFilter. This is consistent with the overarching effort to enhance row ID handling throughout the system.src/backend/executor/functions.c (1)
778-778: Function signature adaptation for ROWID feature.The code correctly updates the call to
ExecInitJunkFilterto include the newhasrowidparameter (set tofalse), adapting to signature changes introduced as part of the Oracle ROWID feature implementation.src/bin/psql/describe.c (3)
1607-1624: LGTM: Well-structured conditional SQL query for ROWID support.The query is properly modified to include the new
c.relhasrowidcolumn when the server version is 170000 or higher. This ensures backward compatibility with older PostgreSQL versions.
1745-1748: LGTM: Proper version-conditional initialization of hasrowid.The code correctly initializes the
hasrowidmember based on server version, setting it to false for versions that don't support the feature.
3570-3574: LGTM: Appropriate conditional display of ROWID information.The code properly checks for verbose mode and ensures the relation is not a materialized view before adding the ROWID status footer. This follows the established pattern for showing additional information in verbose mode.
contrib/pageinspect/heapfuncs.c (1)
247-247: OID handling code removedThe code now unconditionally sets
nulls[12]to true, removing the previous OID handling logic. This change aligns with the overall ROWID feature implementation and removal of old OID handling.src/backend/access/heap/heaptoast.c (5)
176-178: ROWID handling in header calculationThe code now accounts for the ROWID size when calculating the tuple header size, ensuring proper space allocation for tuples that contain a ROWID.
303-305: ROWID handling in new header length calculationConsistent with the earlier change, this section also adds ROWID size to the header length when needed, maintaining consistency in tuple size calculations.
328-330: ROWID preservation during tuple operationsThis code properly preserves the ROWID when creating a new tuple by copying it from the old tuple header, ensuring ROWID values persist through tuple transformations.
522-524: ROWID handling in tuple flatteningThe tuple flattening operation now properly accounts for ROWIDs by adding their size to the header length, ensuring consistency with other tuple operations.
539-541: Setting ROWID during tuple flatteningThis code ensures the ROWID is preserved during the tuple flattening process by copying it from the original tuple to the new tuple header.
src/include/access/tupdesc.h (2)
86-86: New field for tracking ROWID presenceThe addition of the
tdhasrowidfield to theTupleDescDatastructure is a good design choice that allows the system to efficiently track whether tuples have a ROWID attribute in their header, supporting the new ROWID feature.
97-97:✅ Verification successful
Updated function signature to support ROWID handling
The
CreateTupleDescfunction signature has been updated to include two new parameters:tdhasrowidandis_sysattr. This change ensures that tuple descriptors can be properly created with ROWID information.
🏁 Script executed:
#!/bin/bash # Check for any callers of CreateTupleDesc that might need updating with the new parameters rg -A 2 "CreateTupleDesc\(" --type cLength of output: 1042
ROWID Support Update Verified
All usages of
CreateTupleDeschave been updated correctly to pass the newtdhasrowidandis_sysattrparameters. The shell script results confirm:
- In src/backend/bootstrap/bootstrap.c, the function is called with
false, false.- In src/backend/catalog/heap.c, the call uses
tupdesc->tdhasrowidandtrue.- The header file in src/include/access/tupdesc.h reflects the updated signature.
No further changes are necessary on this front.
src/backend/access/common/tupdesc.c (3)
99-99: Initialize tdhasrowid fieldThe
tdhasrowidfield is properly initialized to false in theCreateTemplateTupleDescfunction, providing a safe default value.
113-113: Updated function implementationThe function implementation matches the updated signature in the header file, maintaining consistency.
143-143: Preserve tdhasrowid field in tuple descriptor copiesThe
tdhasrowidfield is properly copied from the source tuple descriptor to the destination tuple descriptor in bothCreateTupleDescCopyandCreateTupleDescCopyConstrfunctions, ensuring this property is preserved when tuple descriptors are duplicated.Also applies to: 186-186
src/backend/commands/indexcmds.c (2)
1093-1093: Good change to support RowIdAttributeNumber indexingThe condition has been modified to allow indexes on the special RowIdAttributeNumber system column, while maintaining the restriction against other system columns. This is a necessary change for implementing Oracle ROWID compatibility.
1111-1113: Properly extends system column exception to expressionsThis change consistently applies the same exception for RowIdAttributeNumber in the context of expressions and predicates. This ensures that indexes using RowIdAttributeNumber in expressions or predicates won't be rejected, which is important for complete ROWID support.
src/backend/access/heap/heapam.c (4)
72-72: Import of sequence.h for ROWID generation.The addition of this include is necessary to access the
nextval_internal()function used for generating ROWID values for tuples in relations that support ROWIDs.
2113-2136: ROWID generation for newly inserted tuples.This code properly generates and sets a ROWID for a new tuple when inserting into a relation with ROWID support. The implementation appropriately:
- Checks if the relation has ROWID support enabled (
relhasrowid)- Verifies the relation has a valid sequence ID (
rd_rowdSeqid)- Generates the next value from the sequence
- Sets the ROWID in the tuple header
Note that the internal assertion is commented out, presumably handled inside
HeapTupleSetRowId().
3190-3198: ROWID preservation during tuple updates.This code ensures that ROWIDs are preserved when updating tuples in relations that have ROWID support. During an update, the ROWID is copied from the old tuple to the new tuple, maintaining ROWID consistency throughout the tuple's lifetime.
8661-8667: ROWID inclusion in replica identity.The change ensures that ROWID is always included in the replica identity tuple when the table has ROWIDs enabled, even if the ROWID is not part of any index. This is a sensible optimization since the space in the logged tuple is allocated anyway, making it beneficial to include this information.
src/backend/parser/parse_clause.c (2)
51-51: Added include for Oracle compatibility.This include adds support for the Oracle compatibility functions needed for the ROWID feature.
3893-3927:❓ Verification inconclusive
New function to support ROWID option handling.
This function checks for the "rowid" option in a list of definitions and decides whether to enable ROWID functionality based on the provided parameters and database compatibility settings.
Key points:
- Scans the definition list for a "rowid" parameter
- Returns error if rowid is found but not allowed
- Uses default GUC (default_with_rowids) when in Oracle compatibility mode
- Otherwise defaults to false (no ROWID)
Let's confirm the current database compatibility settings and check where else this function is called:
🏁 Script executed:
#!/bin/bash # Find where interpretRowidOption is being called grep -r "interpretRowidOption" --include="*.c" . # Check how compatible_db and default_with_rowids are defined/initialized grep -r "compatible_db\s*=" --include="*.c" . grep -r "default_with_rowids" --include="*.c" .Length of output: 8498
ROWID Option Handling – Verification Update
- The new function
interpretRowidOptionscans the provided definition list for a "rowid" parameter, returning its boolean value if found and allowed, or raising an error if not allowed.- When the "rowid" option isn’t explicitly specified, the function falls back to a default: it returns the value of
default_with_rowidsifcompatible_dbequalsDB_ORACLE; otherwise, it returns false.- Verification shows that this function is invoked in multiple modules (e.g.,
createas.c,tablecmds.c, andparse_utilcmd.c), confirming its intended usage across the codebase.- Global variables are defined as expected:
compatible_db(initialized toPG_PARSERinivy_guc.c) anddefault_with_rowids(also inivy_guc.c).- Note: There is some variation in Oracle compatibility checks across the codebase. For instance, while this function uses
DB_ORACLE, other modules sometimes comparecompatible_dbagainstORA_PARSER. Please verify that this difference is intentional and consistent with the overall compatibility strategy.src/backend/executor/execMain.c (2)
1001-1003: Ensure consistent handling oftdhasrowidin junk filtering.
The added parametertupType->tdhasrowidin the call toExecInitJunkFilterensures that row IDs are accounted for in the junk attributes. Please confirm that all top-level plan nodes producing tuples for this junk filter either set or ignoretdhasrowidappropriately, so that no row ID information is erroneously discarded or injected.Would you like a search script to locate all invocations of
ExecInitJunkFilterto guarantee consistency across the codebase?
1364-1395: Validate correct usage ofExecContextForcesRowId.
This newly introduced function succinctly queries multiple flags (EXEC_FLAG_WITH_ROWID,EXEC_FLAG_WITHOUT_ROWID) and inspects the current result relation to determine whether row IDs should be present. The approach is straightforward; however, consider these points:
- If multiple result relations exist, returning early on the first
infomight skip other potential row ID contexts. Verify this aligns with design intentions.- Ensure that no data race occurs if additional threads or parallel workers modify the state’s flags or result relations concurrently.
- Unit tests confirming the function’s return for each bitwise combination of flags would further assure correctness.
contrib/ivorysql_ora/preload_ora_misc.sql (3)
1-7: Check security implications of granting wide privileges.
Granting usage onsysto public and creating adualtable accessible to everyone can be acceptable for Oracle compatibility. However, be aware that this can give all roles knowledge of the system schema. If more restrictive policies are desired, refine these grants.
8-10: No functional changes; only comments.
No concerns; skipping further review.
11-13: Validate new composite types for row IDs.
Bothsys.rowidandsys.urowidappear consistent with the row-identifier feature. Ensure that OID usage forrowoidis the intended approach and thatrowno(bigint) is sufficient for any future expansions, like partition or segment-based row addressing.src/backend/bootstrap/bootstrap.c (1)
623-623: Check additional boolean parameters for tuple descriptor creation.
The change toCreateTupleDesc(numattr, attrtypes, false, false)clarifies that ROWID support is currently off by default in bootstrap mode. Verify that future additions requiring row IDs (or sysattrs) in bootstrap catalogs are handled accordingly.src/backend/parser/parse_expr.c (1)
1105-1120:❓ Verification inconclusive
Validate casting logic for right-hand operand when left expression is of type ROWID.
The new code correctly detects when the left expression is a ROWID in Oracle mode and forces the right expression into a
TypeCasttosys.rowidif it isn’t already a cast. However, consider verifying the following:
- Schema name consistency: Ensure
"sys"is the correct & expected schema name for therowidtype across all Oracle compatibility code.- Double-casting scenarios: If the right expression is already of type ROWID, but wrapped in some other expression (e.g., nested casts or functions), forcing another cast could either be redundant or prevent some valid expression. Moving the
nodeTag(rexpr) != T_TypeCastcheck earlier might help refine this logic.- Test coverage: Add or update tests to cover various cases, such as when the right expression is already
rowid, or when it’s a different type (e.g., text) that needs casting.To confirm there are no missed cases, please run a search for other sites referencing
ROWIDOIDand see whether additional adjustments are needed:
🏁 Script executed:
#!/bin/bash # Searching entire codebase for ROWIDOID references rg "ROWIDOID" -A 5 -B 5Length of output: 2778
Action Required: Verify Oracle RowID Casting Logic
The updated logic appropriately wraps the right expression with a type cast to
sys.rowidwhen the left-hand expression is identified as a ROWID in Oracle compatibility mode. To ensure this logic covers all edge cases, please verify the following:
- Schema Name Consistency: Confirm that using
"sys"as the schema qualifier for the rowid type is consistent and correct across all Oracle compatibility code.- Double-Casting Scenarios: Review whether the simple check
nodeTag(rexpr) != T_TypeCastadequately prevents redundant or improper casts—especially in cases where nested casts or function wrappers might otherwise cause double-casting.- Test Coverage: Ensure tests comprehensively cover scenarios where:
- The right expression is already a rowid (or wrapped in nested casts/functions).
- The right expression is of a different type (e.g., text) that requires explicit casting.
src/backend/executor/nodeSubplan.c (2)
1035-1035: ExecTypeFromTL parameter update for row ID supportThe function call has been updated to include the new
hasrowidparameter set tofalse, which aligns with the Oracle ROWID feature implementation. This ensures that the left side tuple descriptor does not include row ID information.
1043-1043: ExecTypeFromTL parameter update for row ID supportSimilar to the left side, the right side tuple descriptor initialization has been updated to include the
hasrowidparameter set tofalse, maintaining consistency in how subplan tuple descriptors are handled.src/bin/pg_dump/pg_dump.c (6)
6810-6810: Relevant variable addition for ROWID supportThe new variable
i_relhasrowidappropriately tracks the column index for the relhasrowid field in the query result.
6873-6879: Well-implemented version checking for backward compatibilityGood implementation of version-specific query construction. The code correctly handles backward compatibility by:
- Using "false AS relhasrowid" for PostgreSQL versions below 17.0
- Using the actual "c.relhasrowid" column for version 17.0 and above
This ensures pg_dump works properly against both older and newer PostgreSQL versions.
7032-7032: Proper column index lookupThe code correctly retrieves the column index for "relhasrowid" consistent with the pattern used for other columns.
7090-7090: Correct population of table info structureThis line appropriately populates the relhasrowid field in the tblinfo structure, converting PostgreSQL's text boolean representation ("t"/"f") to a C boolean value.
16337-16338: Proper SQL syntax for ROWID tablesThe code correctly adds " WITH ROWID" to the table creation SQL when the source table has ROWID enabled. This is placed at the appropriate position in the statement, before reloptions.
17567-17568:❓ Verification inconclusive
Verify ROWID and sequence interaction
This early return prevents sequences from being dumped for tables with ROWID enabled.
Please verify this behavior is intended and documented. Does ROWID functionality replace or conflict with sequence behavior in some way? Consider adding a comment explaining why tables with ROWID skip sequence dumping.
🏁 Script executed:
#!/bin/bash # Check if there are any comments or documentation about ROWID and sequences rg -i "rowid.*sequence|sequence.*rowid" --type c --type sqlLength of output: 148
Action: Confirm ROWID Handling in Sequence Dumping
The early return at
if (tbinfo->relhasrowid)stops sequences from being dumped for tables with ROWID enabled. Our search only uncovered a hint in src/backend/utils/misc/ivy_guc.c (a gettext reference) and no clear documentation explaining the relationship between ROWID and sequences. Please verify that this behavior is intentional and that ROWID indeed supersedes or replaces sequence functionality. If confirmed, add an inline comment insrc/bin/pg_dump/pg_dump.cclarifying why tables with ROWID bypass sequence dumping to aid future maintainers.src/oracle_test/regress/sql/ora_rowid.sql (1)
1-264: Well-structured and comprehensive test file for the Oracle ROWID feature.This test file provides thorough coverage of the ROWID functionality, systematically testing:
- GUC parameter configuration
- ALTER TABLE commands for ROWID
- CREATE TABLE WITH ROWID syntax
- ROWID data type handling
- Partitioned tables with ROWID
- ROWID indexing
- Table inheritance (LIKE)
- Temporary and Global Temporary Tables
- Views with ROWID
The test cases build on each other logically and cover both successful operations and expected error cases.
contrib/tablefunc/sql/ivy_tablefunc.sql (1)
14-14: Correctly renamed 'rowid' to 'orarowid' to avoid conflicts with system column.All occurrences of the 'rowid' column name have been systematically renamed to 'orarowid' throughout the file. This is necessary to prevent conflicts with the new ROWID pseudo-column that's being introduced as a system column. The changes maintain the original functionality while ensuring compatibility with the ROWID feature.
Also applies to: 17-33, 46-56, 62-62, 79-117, 167-167
src/backend/commands/sequence.c (2)
218-218: Correctly propagating ROWID association to sequence statement.This change ensures that when a sequence is defined, the ROWID association flag is properly passed from the sequence definition to the statement structure.
2455-2472: Enhanced sequence ownership checks for Oracle compatibility.This change improves ownership handling for sequences in Oracle compatibility mode. The new logic:
- For Oracle mode (DB_ORACLE), verifies the sequence name differs from the attribute name before fetching the attribute number
- For PostgreSQL mode (DB_PG), maintains the original behavior
This ensures correct sequence-to-column relationships in Oracle compatibility contexts.
src/oracle_test/regress/GNUmakefile (3)
82-82: Correctly added serial_schedule to regression test files.The serial_schedule file is now included in the regress_data_files, ensuring it's properly installed with the test files.
128-129: Enhanced oracle-installcheck target to run both serial and parallel tests.The oracle-installcheck target now runs both test schedules:
- First the serial_schedule (which includes ROWID tests)
- Then the parallel_schedule
Both use port 1521 and limit connections to 1 for consistent test execution.
144-144:✅ Verification successful
Updated bigtest target to include serial tests.
The bigtest target now also runs the serial_schedule tests with max-connections=1, ensuring the ROWID tests are included in comprehensive testing.
🏁 Script executed:
#!/bin/bash # Verify that serial_schedule includes the ora_rowid test if [ -f "src/oracle_test/regress/serial_schedule" ]; then grep -q "ora_rowid" src/oracle_test/regress/serial_schedule && echo "ora_rowid test is included in serial_schedule" || echo "WARNING: ora_rowid test not found in serial_schedule" else echo "serial_schedule file not found" fiLength of output: 213
Approved: Serial tests now include the ora_rowid test
- The updated bigtest target correctly invokes the serial_schedule tests with
--max-connections=1.- Verification confirms that the
ora_rowidtest is present in thesrc/oracle_test/regress/serial_schedulefile.src/include/nodes/parsenodes.h (4)
2392-2394: New ROWID-related enum values for AlterTableType.These enum values support the Oracle ROWID feature implementation by providing the necessary parser operations for adding and dropping ROWIDs in table alterations. The internal
AT_AddRowidsRecurseoption suggests proper handling of inheritance hierarchy.
2456-2456: Flag to identify ROWID commands in AlterTableCmd structure.The addition of the
is_rowidflag allows the parser to properly identify and process commands related to ROWID operations during ALTER TABLE statement processing.
2681-2681: Supporting ROWID creation at table definition time.The
with_rowid_seqfield added to the CreateStmt structure enables tables to be created with ROWID support from the beginning, not just through later alteration.
3143-3143: ROWID sequence creation support.This field in the CreateSeqStmt structure identifies sequences that are specifically created to support the ROWID feature, allowing different handling from regular sequences.
src/backend/oracle_parser/ora_gram.y (12)
792-792: Addition of "ROWID" to keyword listGood addition of "ROWID" to the list of keywords, ensuring it's properly recognized in the parser grammar.
2529-2537: Proper validation for "rowid" column name conflictsThe code correctly validates that users cannot add a column named "rowid" as it conflicts with a system column name. The usage of
castNodeand error reporting is appropriate.
2543-2551: Proper validation in alternate ADD column syntaxGood implementation of the same validation logic for the parenthesized column definition syntax. This ensures consistent behavior across different ways of adding columns.
2556-2565: Proper validation with IF NOT EXISTS clauseGood implementation of the validation check in the IF NOT EXISTS variant of ADD column.
2570-2579: Proper validation with explicit COLUMN keywordValidation is correctly implemented when using the explicit COLUMN keyword variant of the ADD syntax.
2584-2593: Proper validation with COLUMN IF NOT EXISTSThe validation is correctly implemented for the combined COLUMN IF NOT EXISTS syntax, maintaining consistency with the other variants.
2804-2805: Proper protection for DROP COLUMN IF EXISTSGood implementation of the check preventing users from dropping the system "rowid" column when using the IF EXISTS clause.
2817-2818: Proper protection for DROP COLUMNGood implementation of the check preventing users from dropping the system "rowid" column with the standard DROP COLUMN syntax.
2923-2936: New ALTER TABLE commands for ROWID supportGood addition of new ALTER TABLE commands for enabling and disabling ROWID support:
- SET WITH ROWID creates an AlterTableCmd with AT_AddRowids subtype
- SET WITHOUT ROWID creates an AlterTableCmd with AT_DropRowids subtype
These map correctly to the internal command subtypes needed for the feature.
4882-4883: Support for WITH/WITHOUT ROWID in creation optionsGood implementation of ROWID options in the OptWith rule, allowing specification of ROWID support during table creation. The implementation correctly maintains the pattern used for other options like WITH OIDS.
20117-20117: Addition of ROWID to unreserved keywordsGood addition of ROWID to the list of unreserved keywords, allowing it to be used in SQL statements.
20830-20830: Addition of ROWID to another keyword listProper addition of ROWID to another keyword list, ensuring consistent recognition across different contexts in the grammar.
src/pl/plisql/src/pl_exec.c (4)
7413-7420: Addition of variables for ROWID handling in Oracle compatibility modeAdded new variables to support the handling of Oracle ROWID variables when moving rows from fields. These variables will store tuple information extracted from ROWID values.
7531-7548: Implementation of ROWID variable handling logicAdded a new condition to differentiate between ROWID variables and regular variables when assigning values. This enables Oracle compatibility by using the values extracted from ROWID tuples.
7608-7612: Proper cleanup of allocated memoryGood practice to free allocated memory for
r_valuesandr_nullsafter use to prevent memory leaks.
7456-7456:⚠️ Potential issueMissing null check before operating on tuphd
The code doesn't verify that
tuphdis non-null before operating on it in line 7458. This could lead to a segmentation fault.-if (tuphd) +if (tuphd != NULL)Likely an incorrect or invalid review comment.
src/backend/utils/misc/ivy_guc.c (3)
40-41: LGTM: Clear variable declarations for ROWID featureThese new global variables support the ROWID feature - a boolean flag to control automatic addition of ROWID columns and an integer to control sequence caching for ROWID values. The default values (false and 20) seem reasonable.
143-152: LGTM: Good GUC parameter structure for default_with_rowidsThe configuration entry is properly categorized as a USERSET parameter under DEVELOPER_OPTIONS with appropriate flags to hide it from general users. The description is clear and concise.
204-213:✅ Verification successful
LGTM: Well-structured GUC parameter for rowid_seq_cache
The configuration entry for the cache size has appropriate constraints (min=1, max=60000) and is properly categorized. The description clearly explains its purpose.
Note: Static analysis reported a syntax error on line 213. This appears to be a false positive, but you may want to verify there are no hidden characters or other syntax issues.
🏁 Script executed:
#!/bin/bash # Check the syntax of the file with gcc preprocessor gcc -E -P src/backend/utils/misc/ivy_guc.c -o /dev/null 2>&1 || echo "Syntax error confirmed"Length of output: 206
LGTM – No Actual Syntax Issue Detected
The syntax error reported on line 213 appears to be a false positive. The gcc preprocessor wasn’t executed because gcc isn’t available in the current environment, so the “Syntax error confirmed” output stems from that limitation rather than from the code itself. A manual inspection of theivy_guc.cfile (lines 204–213) reveals no hidden characters or anomalies that would affect syntax. The configuration forrowid_seq_cacheis properly defined with the correct constraints and flags.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 213-213: syntax error
(syntaxError)
src/include/access/htup_details.h (4)
193-193: LGTM: Clear flag definition for ROWID fieldThe HEAP_HASROWID flag is appropriately defined to indicate that a tuple has a ROWID field.
545-551: LGTM: Well-implemented getter macro for ROWIDThe HeapTupleHeaderGetRowId macro correctly checks for the HEAP_HASROWID flag before attempting to access the ROWID field. The return of -1 for tuples without a ROWID is a sensible default.
553-557: LGTM: Safe setter macro with assertionThe HeapTupleHeaderSetRowId macro includes an appropriate assertion to prevent setting a ROWID on a tuple that doesn't have the HEAP_HASROWID flag set.
559-563: LGTM: Convenient wrapper macrosThese wrapper macros provide a simpler interface for getting and setting ROWIDs on a HeapTuple, similar to other accessor patterns in the codebase.
src/backend/executor/execTuples.c (5)
73-73: LGTM: Added hasrowid parameter to function signatureThe hasrowid parameter allows the function to handle ROWID columns appropriately.
1844-1855: LGTM: Well-implemented ROWID context checkingThis code correctly determines whether the current execution context requires a ROWID, with good code structure that isolates the decision logic.
2037-2040: LGTM: Updated function to pass hasrowid parameterThe ExecTypeFromTL function now correctly propagates the hasrowid parameter to the internal implementation.
2049-2052: LGTM: Updated ExecCleanTypeFromTL function signatureThis function now also correctly handles the hasrowid parameter, ensuring consistent behavior across the tuple descriptor creation functions.
2055-2067: LGTM: Setting hasrowid flag in tuple descriptorThe implementation correctly sets the tdhasrowid field in the tuple descriptor based on the passed parameter.
src/backend/access/common/heaptuple.c (4)
69-70: Ensure that these includes are necessary and used consistently.
The newly added headers (typcache.handfuncapi.h) likely support row-type lookups and function utilities for the ROWID feature. This appears valid given the usage in the new ROWID-related code.
487-487: MarkingRowIdAttributeNumberas never null is consistent with other system attributes.
This aligns with the design choice that system attributes, including the new rowid, are guaranteed to be present.
1189-1190: Adding 8 bytes for rowid storage is valid.
Reserving space for a 64-bit rowid attribute in the tuple layout aligns with your design.
1530-1531: Reserving 8 bytes in minimal tuple for rowid is consistent.
This follows the same approach as for regular tuples, ensuring space for the new rowid field.src/backend/catalog/heap.c (10)
77-78: No issues found.These additional includes for
"utils/ora_compatible.h"and"utils/guc.h"align with the new Oracle-compatibility changes and GUC definitions.
234-249: Looks good, but verify alignment for varlena usage.The new "rowid" attribute shares similarities with "ctid" in using
TYPALIGN_SHORT. While this is consistent with how system attributes like "ctid" are declared, you may want to confirm that the custom typeROWIDOIDtruly requires only short alignment, especially since it hasattlen = -1(varlena). Otherwise, the definition looks correct for providing an Oracle ROWID pseudo-column.Would you like me to run a quick check to confirm that
ROWIDOIDindeed uses short alignment in the rest of the codebase?
251-251: No concerns.The new attribute pointer,
&a7, is correctly added to theSysAtt[]array, ensuring the ROWID pseudo-column is recognized as a system attribute.
890-890: Verify usage of 'tdhasrowid'.Ensure that passing
tupdesc->tdhasrowidas the third argument toCreateTupleDesccorrectly reflects the extended signature that handles the ROWID pseudo-column. Confirm that further usage of this tuple descriptor properly distinguishes system attributes.
952-952: Relhasrowid storage looks consistent.Storing
relhasrowidin the pg_class tuple aligns with tracking the new ROWID pseudo-column. Just verify that schema upgrades and any existing tools recognize the new field.
1337-1341: Consistent assignment of ROWID flags.Copying
tdhasrowidandrelhasrowidfrom the source descriptor into the newly created relation ensures the metadata accurately reflects the presence of the ROWID pseudo-column.
1722-1727: Reassess the removal of system attributes.Allowing direct deletion of a negative-numbered (system) attribute may have ramifications if "rowid" is critical for internal operations. Confirm that dropping ROWID this way won't generate inconsistencies or break Oracle-compatibility assumptions.
1728-1730: Separate handling of system vs. user attributes.These lines structure the else-case for non-system attributes after the block for system attributes. No immediate concerns beyond verifying the logic in the surrounding code.
1779-1780: Appropriate catalog tuple update.Ensures the catalog reflects the column's new dropped state for non-system attributes. Looks correct given the prior logic.
1789-1790: Cleans up associated statistics for dropped user attributes.Skipping stats removal for negative (system) attributes is sensible, as system columns generally lack pg_statistic entries. No issues found.
src/backend/commands/tablecmds.c (24)
83-83: No issues observed with the new include statement.
363-363: The additional parameter to track parent rowids looks sound.
892-893: PropagatingparentRowIdCountthrough MergeAttributes is consistent.
902-907: Confirm the semantics of applying rowids to sequences.It’s unusual to associate rowids with a sequence. Please verify that this behavior is intentional and tested.
1284-1317: Check for existing or duplicate rowid indexes before creating a new one.Ensure we don’t unintentionally create multiple rowid indexes if the user already defined one. Would you like a script to search for potential index checks in the repository?
2528-2533: Storing the parent rowid count is well-integrated.
2721-2723: Accurately increments parent rowid count for inherited tables.
3097-3097: *Correctly sets supRowIdCount to the computed parent count.
4567-4567: Requiring AccessExclusiveLock for adding rowids is appropriate.
4598-4598: Requiring AccessExclusiveLock for dropping rowids is appropriate.
5124-5167: Validate rowid usage on foreign tables.It might be necessary to confirm rowid is meaningfully supported by all relevant FDWs. Otherwise, restricting rowid usage could be safer.
5536-5560: Delegating work to ATExecAddColumn for AddRowids looks correct.
6202-6209: Preserving tdhasrowid during rewrite is necessary.
6740-6744: Providing distinct command tags for Add/Drop rowid is clear.
7216-7221: Appropriately assigns the subtree command subtype for rowid additions.
7240-7240: Straightforward extraction of theis_rowidflag from the command.
7305-7310: Ensures child tables cannot conflict on rowid attribute number.
7386-7396: Assigning rowid’s fixed attribute number is correct.
7420-7423: Properly reflects rowid presence inpg_class.
7508-7509: Skipping system attributes in default expression handling makes sense.
7577-7583: Forcing a table rewrite on rowid addition ensures consistency.
7631-7633: Propagating rowid state to child commands is consistent.
9427-9428: Prevents dropping system columns unless it’s the rowid.
9563-9632: Verify that forcibly removing rowid sequence won’t break existing dependencies.If others reference this sequence, a forced removal might be problematic. Consider using standard cascade/restrict semantics instead of blindly deleting dependencies.
src/backend/parser/parse_utilcmd.c (6)
88-88: Field addition appears consistent.The new boolean field
hasrowidclearly conveys whether a relation will include a ROWID column. This is a straightforward addition and aligns with surrounding flag fields (e.g.,relhasrowid).
182-186: Local variable additions are fine.These new local variables (
like_found,seqstmt,altseqstmt,relnamelist) are used cohesively to handle rowid-related sequence creation when processing LIKE clauses. No issues found with their declarations or usage.
265-265: Usage of interpretRowidOption seems appropriate.The call to
interpretRowidOption(stmt->options, !cxt.isforeign)correctly stores the result incxt.hasrowid, ensuring the new table's ROWID feature is toggled based on the user options.
304-310: Rowid column name rejection aligns with Oracle compatibility.The code prevents a user-defined column named "rowid" when
compatible_db == DB_ORACLE, which is consistent with restricting a reserved column name.
343-348: Auto-adding ROWID option for LIKE is correct.When a LIKE clause is found and
cxt.hasrowidis true, adding"rowid"tostmt->optionsensures the new table inherits the ROWID characteristic.
349-414: Sequence creation for ROWID is logically consistent.Creating and attaching a new sequence for ROWID, setting cache options, and marking ownership are all consistent with the approach for identity columns. If external checks ensure valid
rowid_seq_cache, the logic is sound and effectively handles the sequence lifecycle.src/include/executor/executor.h (4)
71-73: Execution flags for ROWID handling added correctly.The introduction of
EXEC_FLAG_WITH_ROWIDandEXEC_FLAG_WITHOUT_ROWIDflags provides explicit control over whether ROWIDs should be included in returned tuples. The repositioning ofEXEC_FLAG_WITH_NO_DATAto a higher value (0x0100) makes room for these new flags while maintaining backward compatibility.
171-172: Function signature updated to support ROWID functionality.The
ExecInitJunkFilterfunction prototype has been correctly modified to include the newhasrowidparameter, which allows the junk filter to properly handle ROWIDs in tuples.
224-224: New helper function for ROWID context determination added.The
ExecContextForcesRowIdfunction will help determine whether the execution context requires including ROWIDs in result tuples, which is essential for the Oracle ROWID compatibility feature.
501-502: Tuple descriptor functions updated to support ROWID inclusion.The
ExecTypeFromTLandExecCleanTypeFromTLfunction signatures have been properly updated to include thehasrowidparameter, ensuring that tuple descriptors can be correctly created with or without ROWID attributes.src/backend/utils/cache/relcache.c (11)
448-449: LGTM: Proper initialization of tuple descriptor rowid flag.Correctly initializes the tuple descriptor's
tdhasrowidfield from the relation form, ensuring consistency between the relation and its descriptor.
92-92: LGTM: Necessary header inclusion for new feature.The inclusion of
access/heapam.his appropriate for supporting the new rowid functionality which interacts with heap access methods.
1239-1245: LGTM: Proper conditional handling for rowid sequence ID.Good implementation of the rowid sequence ID initialization. The code correctly checks both the relation's
relhasrowidflag and that the relation is of an appropriate kind (regular table or partitioned table) before retrieving the sequence ID.
539-539: LGTM: Consistent initialization of rowid flag.Ensuring the tuple descriptor's
tdhasrowidfield matches the relation'srelhasrowidfield during relation building is important for consistency.
2012-2014: LGTM: Proper initialization of tuple descriptor in template creation.Correctly initializes the
tdhasrowidfield when creating a template tuple descriptor, ensuring the rowid status is properly propagated.
4486-4486: LGTM: Updated API to pass through rowid flag.The
BuildHardcodedDescriptorfunction signature has been properly updated to include thehasrowidparameter, ensuring consistent handling of rowid information.
934-942: LGTM: Proper catalog lookup for rowid sequence.The implementation properly constructs scan keys to look up the rowid sequence in the pg_class catalog using the relation's namespace and owner. This ensures that the sequence is correctly associated with the relation.
2001-2001: LGTM: Proper initialization of rowid flag in relation form.Correctly initializes the relation's
relhasrowidfield from the tuple descriptor'stdhasrowidfield during relation creation.
1132-1141: LGTM: Updated function calls with rowid parameter.All calls to
formrdeschave been properly updated to include the newhasrowidparameter, ensuring consistent handling of rowid information across various catalog relations.
4190-4197: LGTM: Updated function calls with rowid parameter.The calls to
formrdescfor the local critical relations have been properly updated to include the newhasrowidparameter, maintaining consistency with the new API.
896-897: LGTM: Updated function signature to support new feature.The
formrdescfunction signature has been properly updated to include thehasrowidparameter, enabling support for the Oracle rowid feature.
| if (tupleDescriptor->tdhasrowid) | ||
| td->t_infomask = HEAP_HASROWID; |
There was a problem hiding this comment.
Avoid overwriting existing infomask bits.
Using td->t_infomask = HEAP_HASROWID discards other flags (e.g., HEAP_HASNULL). Instead, combine it with existing bits:
- td->t_infomask = HEAP_HASROWID;
+ td->t_infomask |= HEAP_HASROWID;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (tupleDescriptor->tdhasrowid) | |
| td->t_infomask = HEAP_HASROWID; | |
| if (tupleDescriptor->tdhasrowid) | |
| td->t_infomask |= HEAP_HASROWID; |
| if (tupleDescriptor->tdhasrowid) | ||
| tuple->t_infomask = HEAP_HASROWID; |
There was a problem hiding this comment.
Use bitwise OR to preserve existing infomask flags.
As with regular tuples, using assignment overwrites flags like HEAP_HASNULL. Update to:
- tuple->t_infomask = HEAP_HASROWID;
+ tuple->t_infomask |= HEAP_HASROWID;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (tupleDescriptor->tdhasrowid) | |
| tuple->t_infomask = HEAP_HASROWID; | |
| if (tupleDescriptor->tdhasrowid) | |
| tuple->t_infomask |= HEAP_HASROWID; |
| if (strcmp(NameStr(attr->attname), "rowid") == 0 && compatible_db != DB_ORACLE) | ||
| continue; | ||
|
|
There was a problem hiding this comment.
Possible reversed condition.
If the intention is to reserve "rowid" as a system attribute only in Oracle-compatible mode, the logic seems inverted. Currently, if the database is not Oracle-compatible, then we skip the collision check for "rowid." Double-check that this matches the intended behavior regarding creation of a user-defined "rowid" column under non-Oracle modes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pl/plisql/src/pl_exec.c (1)
7452-7456: Consider avoiding memory leaks from pstrdupThe string comparison here uses
pstrdupwhich creates a copy that's never freed. This could cause memory leaks during repeated executions.- if (strcmp(pstrdup(NameStr(tupdesc->attrs[i].attname)), "rowid") == 0) + if (strcmp(NameStr(tupdesc->attrs[i].attname), "rowid") == 0)This was specifically mentioned in the previous review as a suggested improvement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/backend/parser/parse_utilcmd.c(8 hunks)src/backend/utils/cache/relcache.c(17 hunks)src/pl/plisql/src/pl_exec.c(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/backend/utils/cache/relcache.c
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: oracle_pg_regression (ubuntu-latest)
- GitHub Check: meson_build (ubuntu-latest)
- GitHub Check: contrib_regression
- GitHub Check: oracle_regression (ubuntu-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: pg_regression (ubuntu-latest)
🔇 Additional comments (12)
src/backend/parser/parse_utilcmd.c (8)
88-88: Introducehasrowidfield for row identifier tracking.The addition of
hasrowidin theCreateStmtContextstruct provides a clear way to propagate row identifier features through the statement creation flow. No issues noted with the placement or usage.
182-185: Initialize variables forLIKEand row ID sequence context.The new variables
like_found,seqstmt,altseqstmt, andrelnamelistcleanly track the presence of a LIKE clause and the state of sequence creation. Their use later in the function is consistent and well-scoped.
265-265: CallinterpretRowidOptionfor non-foreign tables.Ensuring rowid options are interpreted only for non-foreign tables is a neat safety check. The condition
!cxt.isforeigncleanly avoids rowid logic where unsupported.
280-291: Determinehasrowidbased on inherited relations.Using logical OR to aggregate the presence of
relhasrowidin any parent ensures correct inheritance of the row identifier property. This resolves potential overwriting issues by earlier code versions.
304-310: Block Oracle-specificrowidcolumn name conflicts.The check that disallows an explicitly named "rowid" column when in Oracle compatibility mode appropriately prevents collisions with the system column. The error message and path are concise.
318-318: Mark that a LIKE clause was found.Setting
like_found = trueensures subsequent logic can differentiate between normal and LIKE-based table creation. Straightforward and effective.
343-348: Inherit rowid setting through LIKE clauses.Silent propagation of the rowid property by prepending a new defElem indicates a smooth approach to carrying rowid over from referenced tables. The logic is simple and clear.
349-413: Generate and attach a rowid sequence for the new table.This block parallels the existing sequence-creation pattern used for serial/identity columns, customizing it with
with_rowidand an optional cache. It thoroughly covers ownership (viaALTER SEQUENCE ... OWNED BY), ensuring the sequence remains tied to the table’s rowid column. No concerns identified.src/pl/plisql/src/pl_exec.c (4)
7413-7421: Implementation of Oracle rowid featureThe code adds variables needed to support Oracle rowid functionality in the tuple handling mechanism. This allows PL/iSQL to properly process Oracle's ROWID data type.
7446-7474: Oracle rowid handling logicThis block checks if the current record's data type is an Oracle ROWID type and if running in Oracle compatibility mode. If so, it correctly processes the rowid by:
- Searching for an attribute named "rowid" in the tuple
- If found, extracting the heap tuple header and related metadata
- Preparing structures to hold rowid data
The implementation properly initializes
tuphdto NULL before the search, addressing the initialization concern from the previous review.
7531-7539: Processing rowid attribute valuesThis code correctly handles Oracle rowid values during field assignment, using the specially prepared rowid data when the record is recognized as a rowid type. The condition checks both the rowid flag and valid index bounds for safety.
7608-7611: Proper cleanup of allocated resourcesThe code properly cleans up allocated memory for rowid processing, freeing
r_valuesandr_nullsarrays if they were allocated. This prevents memory leaks.
2 participants
Summary by CodeRabbit