Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[YSQL] fix ysql_dump/Postgres so pg_class OIDs are preserved #23304

Closed
1 task done
mdbridge opened this issue Jul 29, 2024 · 0 comments
Closed
1 task done

[YSQL] fix ysql_dump/Postgres so pg_class OIDs are preserved #23304

mdbridge opened this issue Jul 29, 2024 · 0 comments
Assignees
Labels
area/ysql Yugabyte SQL (YSQL) kind/new-feature This is a request for a completely new feature priority/medium Medium priority issue

Comments

@mdbridge
Copy link
Contributor

mdbridge commented Jul 29, 2024

Jira Link: DB-12226

Description

Today in YugabyteDB using Postgres 11, ysql_dump preserves OIDs for pg_enum but not for pg_classes. I think it may preserve OIds for pg_types already, but I'm not sure.

This task is to make it preserve OIDs for all of these three "kinds" of OIDs.

We need this in order to do sequence replication for xCluster: when we replicate the sequences_data table, it contains sequence (pg_class) OIDs and we do not want to have to translate these tween the source and target universes.

We need the pg_types as well for things like user types.

This preservation should be when the --include-yb-metadata flag is passed to ysql_dump.

Issue Type

kind/new-feature

Warning: Please confirm that this issue does not contain any sensitive information

  • I confirm this issue does not contain any sensitive information.
@mdbridge mdbridge added area/ysql Yugabyte SQL (YSQL) status/awaiting-triage Issue awaiting triage labels Jul 29, 2024
@yugabyte-ci yugabyte-ci added kind/new-feature This is a request for a completely new feature priority/medium Medium priority issue and removed status/awaiting-triage Issue awaiting triage labels Jul 29, 2024
mdbridge added a commit that referenced this issue Aug 17, 2024
Summary:
This diff modifies ysql_dump and Postgres so all pg_class and pg_type OID are preserved across a backup/restore when the (existing) --include-yb-metadata flag is passed to ysql_dump.

See #23304 for context on why we want to do this.
Jira: DB-12226

Test Plan:
Jenkins

```
ybd  --java-test 'org.yb.pgsql.TestYsqlDump#ysqlDumpAllWithYbMetadata' >& /tmp/generic.mdl.log
ybd  --java-test 'org.yb.pgsql.TestYsqlDump#ysqlDumpWithYbMetadata' >& /tmp/generic.mdl.log
ybd  --java-test 'org.yb.pgsql.TestYsqlDump#ysqlDumpColocatedDB' >& /tmp/generic.mdl.log
ybd  --java-test 'org.yb.pgsql.TestYsqlDump#ysqlDumpLegacyColocatedDB' >& /tmp/generic.mdl.log
ybd  --java-test 'org.yb.pgsql.TestYsqlDump#ysqlDumpWithoutYbMetadata' >& /tmp/generic.mdl.log
```

Added two new tests to verify OIDs are preserved properly:
```
ybd --cxx-test yb-backup-cross-feature-test --gtest_filter '*.TestPreservingPgEnumOids' --test-args '--verbose_yb_backup' >& /tmp/generic.mdl.log
ybd --cxx-test yb-backup-cross-feature-test --gtest_filter '*.TestPreservingPgTypeAndClassOids' --test-args '--verbose_yb_backup' >& /tmp/generic.mdl.log
```

Reviewers: myang

Reviewed By: myang

Subscribers: yql, ybase

Differential Revision: https://phorge.dev.yugabyte.com/D36675
mdbridge added a commit that referenced this issue Aug 17, 2024
…class OIDs

Summary:
The previous diff of this stack, https://phorge.dev.yugabyte.com/D36675, changed ysql_dump/Postgres so it emits information about the pg_class OIDs of objects and at restore time verifies that every object that need such an OID has one provided.

Unfortunately, this breaks backward compatibility: if we attempt to restore a dump made before that diff, it fails because those dumps don't have information about every object that needs such OID.

This diff fixes this by making the check only applied if a new GUC, yb_ignore_heap_pg_class_oids, is turned on.  The new dump sets this variable in its output but it would obviously not be set in the old dump output.

This let's us restore old backups without problem but would turn off the check for some cases we don't use (e.g., trying to restore output of pg_dump with binary upgrade set).

Test Plan:
Verify preserving all pg_class OIDs works for new version:
```
ybd --cxx-test yb-backup-cross-feature-test --gtest_filter '*.TestPreservingPgEnumOids' --test-args '--verbose_yb_backup' >& /tmp/generic.mdl.log
ybd --cxx-test yb-backup-cross-feature-test --gtest_filter '*.TestPreservingPgTypeAndClassOids' --test-args '--verbose_yb_backup' >& /tmp/generic.mdl.log
```

Testing backward compatibility:
  * spin up a universe without this diff
  * create a sequence (CREATE SEQUENCE foo;)
  * dump out the metadata using YSQL dump:
```
build/latest/postgres/bin/ysql_dump --host=127.0.0.10 --dbname=yugabyte --include-yb-metadata --serializable-deferrable --create --schema-only --file=/tmp/dump
```
  * shut that universe down and start up a new one with the diff
  * restore the metadata:
```
~/code/yugabyte-db/bin/ysqlsh -h 127.0.0.20 --dbname=template1 -c 'DROP DATABASE yugabyte;'
bin/ysqlsh -h 127.0.0.20 --dbname=template1 --file=/tmp/dump
```
  * verify no errors

To ensure check still works, add `SET yb_ignore_heap_pg_class_oids = false;` to that dump after each occurrence of
```
SET yb_binary_restore = true;
```
verify you get an error trying to restore it:
```
COMMENT
 binary_upgrade_set_next_pg_type_oid
-------------------------------------

(1 row)

ysqlsh:/tmp/dump:79: ERROR:  pg_class heap OID value not set when in binary upgrade mode
ysqlsh:/tmp/dump:83: ERROR:  relation "public.mdl_sequence" does not exist
```

Other tests:
```
ybd  --java-test 'org.yb.pgsql.TestYsqlDump#ysqlDumpAllWithYbMetadata' >& /tmp/generic.mdl.log
ybd  --java-test 'org.yb.pgsql.TestYsqlDump#ysqlDumpWithYbMetadata' >& /tmp/generic.mdl.log2
ybd  --java-test 'org.yb.pgsql.TestYsqlDump#ysqlDumpColocatedDB' >& /tmp/generic.mdl.log3
ybd  --java-test 'org.yb.pgsql.TestYsqlDump#ysqlDumpLegacyColocatedDB' >& /tmp/generic.mdl.log4
ybd  --java-test 'org.yb.pgsql.TestYsqlDump#ysqlDumpWithoutYbMetadata' >& /tmp/generic.mdl.log5
```

Reviewers: myang

Reviewed By: myang

Subscribers: xCluster, ybase, yql

Differential Revision: https://phorge.dev.yugabyte.com/D37360
jasonyb pushed a commit that referenced this issue Aug 20, 2024
Summary:
 b8cd4da Fix broken header links in Explore section (#23522)
 cc43b2e [DEVOPS-3048] test automation: Backup unit tests use YBC by default
 43652cc [PLAT-13836] Upgrading python setuptools
 71f5eeb [doc][yba] Note on deleting KMS config (#23527)
 a7061c6 [PLAT-14912] Adding replicated migrate guardrails for subdirectories
 9b21783 [#23286] xCluster: Speedup setup for large table counts
 6d4d8f6 [DEVOPS-3048] test automation: Fix ybc extraction
 15fd362 [PLAT-14951] Add positive interger error message to pitr param step
 c8cbcbf [#23243] docdb: Fix tablet bootstrap stuck when replaying truncate operation
 5f286f5 [PLAT-14976] Make node agent silent parameters more obvious by showing in usage
 68ac66e [#23492] DocDB: Upgrade and Rollback tests
 Excluded: 516ead0 [#23304] xCluster: fix ysql_dump/Postgres so pg_class OIDs are preserved
 Excluded: 16941de [#23304] fix Postgres so old dumps can be loaded that do not have pg_class OIDs
 404075d [#23376] DocDB: Utilities needed for HNSW
 875ccc1 [PLAT-14077] Update /get endpoint to support db scoped replication tables + metrics
 71610b5 [#23536] fix test_macros.h to avoid problems with complaints about capturing variables
 027f0e1 [#23493] xCluster: implement function for scanning sequences_data table
 9103885 [#22462] DocDB: Enable pg_cron tests in TSAN
 7e1f72c [PLAT-14963]Clicking use same/diff replica while DR repair is throwing a permission error for a superadmin user
 b983d56 [PLAT-14595] Ability to change communication ports via edit universe
 a6ee050 [PLAT-13285] Make cloud provider edit retryable

Test Plan: Jenkins: rebase: pg15-cherrypicks

Reviewers: jason, tfoucher

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D37382
mdbridge added a commit that referenced this issue Sep 5, 2024
…so pg_class OIDs are preserved

Summary:
Original commit: 516ead0 / D36675

Note: Summary of what the diff does at the end after the modifications.

Modifications due to standard conflicts that auto resolve could not resolve:

- heap.c
  - function heap_create_with_catalog (two conflicts)
    - master commit 516ead0 changes the logic for when to use OIDs (yb_binary_restore now enables rather than disables)
    - upstream Postgres 37b2764593c073ca61c2baebd7d85666e553928b refactors how RELKIND are handled
    - resolution: apply the just the intended logic change to the upstream Postgres code

- index.c
  - function index_create
    - master commit 516ead0 changes the logic for when to use OIDs (yb_binary_restore now enables rather than disables)
    - YB pg15 55782d5 changes the comment slightly
    - resolution: take both changes (they are on adjacent lines)

- pg_dump.c
  - function dumpCompositeType
    - master commit 516ead0 changes the logic for when to use OIDs (include_yb_metadata now enables)
    - upstream Postgres 6df7a9698bb036610c1e8c6d375e1be38cb26d5f adds a next argument to the call to binary_upgrade_set_type_oids_by_type_oid
    - resolution: make both changes
  - function dumpTableSchema
    - master commit 516ead0 makes a slight optimization
    - YB pg15 180e7ae removes the code that makes us only emit primary keys if our parent is not a partition
    - resolution: take the code from PG 15 as the code being optimized no longer exists
  - function dumpSequence
    - master commit 516ead0 changes the logic for when to use OIDs (include_yb_metadata now enables)
    - upstream Postgres f3faf35f370f558670c8213a08f2683f3811ffc7 removes the code that attempts to preserve pg_type OIDs for sequences
    - resolution: apply both changes

Modifications due to PG 15 changes that require changing in order to keep the intended diff functionality:

- typecmds.c
  - function AssignTypeMultirangeArrayOid
    - upstream Postgres 6df7a9698bb036610c1e8c6d375e1be38cb26d5f adds handling of multi-range data types
    - resolution: add code to turn on code for setting multirange array OIDs when yb_binary_restore is true; this was already partly done by YB pg15 2e57f19

- heap.c
  - function heap_create_with_catalog
    - upstream Postgres 9a974cbcba005256a19991203583a94b4f9a21a9 changes PG dump to also attempt to preserve relfilenodes; this breaks our restore
    - resolution: add code to not require preserving relfilenodes if yb_binary_restore is true

- index.c
  - function index_create
    - upstream Postgres 9a974cbcba005256a19991203583a94b4f9a21a9 changes PG dump to also attempt to preserve relfilenodes; this breaks our restore
    - resolution: add code to not require preserving relfilenodes if yb_binary_restore is true

- relcache.c
  - function RelationSetNewRelfilenode
    - upstream Postgres 9a974cbcba005256a19991203583a94b4f9a21a9 changes PG dump to also attempt to preserve relfilenodes; this breaks our restore
- heap.c
  - function heap_create_with_catalog
    - upstream Postgres 9a974cbcba005256a19991203583a94b4f9a21a9 changes PG dump to also attempt to preserve relfilenodes; this breaks our restore
    - resolution: add code to not require preserving relfilenodes if yb_binary_restore is true

- index.c
  - function index_create
    - upstream Postgres 9a974cbcba005256a19991203583a94b4f9a21a9 changes PG dump to also attempt to preserve relfilenodes; this breaks our restore
    - resolution: add code to not require preserving relfilenodes if yb_binary_restore is true

- pg_dump.c
  - function binary_upgrade_set_pg_class_oids
    - upstream Postgres 9a974cbcba005256a19991203583a94b4f9a21a9 changes PG dump to emit instructions for preserve relfilenodes
    - resolution: make no changes here; the previous changes cause those instructions to be ignored when the script output by ysql_dump is run

Modifications due to expected output needing to be updated:

The following files have been changed to the new output:
  - yb_ysql_dump.data.sql
  - yb_ysql_dump_colocated_database.data.sql
  - yb_ysql_dump_legacy_colocated_database.data.sql
  - yb_ysql_dumpall.data.sql

Noteworthy causes of these changes:
- instructions to preserve relfilenodes
  - PG upstream 9a974cbcba005256a19991203583a94b4f9a21a9 adds code to preserve relfilenodes
- conflict about whether to preserve OIDs for sequences
    - master commit 516ead0 turns on output for setting pg_type OID for sequences
    - upstream Postgres f3faf35f370f558670c8213a08f2683f3811ffc7 removes the code that attempts to preserve OIDs for sequences
    - this was resolved elsewhere in favor of Postgres/removing the preservation
- changes to the formatting of some values
  - YB pg15 ed96733 changes the formatting of some values
- handling of dumping partitioned tables
  - YB pg15 180e7ae changes handling of partitioned tables, using attach instead of create
- changes in database/ysql_dump versions
  - these are dumped in the output and thus changed

Summary of what this diff does:

  * Previously in PG 11 branch, when ysql_dump was called with  --include_yb_metadata, it ran only a subset of the usual Postgres --binary_update functionality
    * the subset ran preserved OIDs of kind "pg_enum" and "pg_type"
    * the code for preserving OIDs of kind "pg_class" was not run
    * because part of this code runs at dump time and part runs at "restore time", ysql_dump when passed this flag starts its output with an instruction to set a GUC, yb_binary_restore; that allows this flag to affect the restore-time behavior
  * the PG 11 branch version of this diff changed this to also run the code for preserving OIDs of kind "pg_class"

  * in slightly more detail, preserving a OID involves two pieces:
    * at dump time, emitting an instruction before the DDL that creates the object that should have the OID instructing the next DDL to use the given OID
      * e.g., when dumping object foo with OID 13 it emits the moral equivalent of:
        * <for next DDL use OID 13>; CREATE foo ...
    * at "restore" time (e.g., when executing the script created by ysql_dump), it honors that instruction when creating the object
      * e.g., the CREATE foo instruction will use the OID 13 because of the preceding instruction
      * there is also a check at restore time that OIDs have been given for all objects of the given "kinds" that are supposed to be preserved
        * e.g., if the dump instructions do a CREATE ENUM without supplying a OID then a check fails

  * This diff itself is a merge of the original diff into PG 15
    * it continues to preserve the now running the code for preserving OIDs of kind "pg_class" part, including for new Postgres 15 code that handles a new object type, multi-range data types
    * Postgres 15 started preserving relfilenodes in addition to OIDs
      * this breaks our current restore code (very roughly, DocDB tables are created with IDs based on OID but accessed using IDs based on relfilenodes)
      * to handle this, this diff also turns off the second half of the preserve relfilenodes code when --include_yb_metadata is passed
        * that is, the code to honor the instructions to preserve relfilenodes, including the code to check that relfilenodes have been supplied, does not run when yb_binary_restore is set
        * the code to emit the instructions at dump time to preserve relfilenodes still runs

  * Why produce the instructions to preserve relfilenodes when we don't intend to honor them?
    * Answer: a current project, allowing DDL during backups, is considering either fixing the code to honor those instructions or just using them as hints on how to matchup tables being imported with the table metadata created by the output of ysql_dump

Rest of original diff summary:

This diff modifies ysql_dump and Postgres so all pg_class and pg_type OID are preserved across a backup/restore when the (existing) --include-yb-metadata flag is passed to ysql_dump.

See #23304 for context on why we want to do this.
Jira: DB-12226

Test Plan:
Jenkins

```
ybd  --java-test 'org.yb.pgsql.TestYsqlDump#ysqlDumpAllWithYbMetadata' >& /tmp/generic.mdl.log
ybd  --java-test 'org.yb.pgsql.TestYsqlDump#ysqlDumpWithYbMetadata' >& /tmp/generic.mdl.log
ybd  --java-test 'org.yb.pgsql.TestYsqlDump#ysqlDumpColocatedDB' >& /tmp/generic.mdl.log
ybd  --java-test 'org.yb.pgsql.TestYsqlDump#ysqlDumpLegacyColocatedDB' >& /tmp/generic.mdl.log
ybd  --java-test 'org.yb.pgsql.TestYsqlDump#ysqlDumpWithoutYbMetadata' >& /tmp/generic.mdl.log
```

Added two new tests to verify OIDs are preserved properly:
```
ybd --cxx-test yb-backup-cross-feature-test --gtest_filter '*.TestPreservingPgEnumOids' --test-args '--verbose_yb_backup' >& /tmp/generic.mdl.log
ybd --cxx-test yb-backup-cross-feature-test --gtest_filter '*.TestPreservingPgTypeAndClassOids' --test-args '--verbose_yb_backup' >& /tmp/generic.mdl.log
```

Reviewers: jason, tfoucher

Reviewed By: jason, tfoucher

Subscribers: smishra, ybase, yql

Differential Revision: https://phorge.dev.yugabyte.com/D37631
mdbridge added a commit that referenced this issue Sep 6, 2024
…loaded that do not have pg_class OIDs

Summary:
Original commit: 16941de / D37360

Modifications due to standard conflicts that auto resolve could not resolve:

- heap.c
  - function heap_create_with_catalog
    - master commit 16941de changes the logic for when to require pg_class heap OIDs (they are now only required when yb_ignore_heap_pg_class_oids is false when in yb_binary_restore mode)
    - PG cherry picks trunk via merge of master commit 516ead0 with upstream Postgres 37b2764593c073ca61c2baebd7d85666e553928b refactors how RELKIND are handled
    - resolution: apply the just the intended logic change to the upstream Postgres code

Rest of original diff summary:

The previous diff of this stack, https://phorge.dev.yugabyte.com/D36675, changed ysql_dump/Postgres so it emits information about the pg_class OIDs of objects and at restore time verifies that every object that need such an OID has one provided.

Unfortunately, this breaks backward compatibility: if we attempt to restore a dump made before that diff, it fails because those dumps don't have information about every object that needs such OID.

This diff fixes this by making the check only applied if a new GUC, yb_ignore_heap_pg_class_oids, is turned on.  The new dump sets this variable in its output but it would obviously not be set in the old dump output.

This let's us restore old backups without problem but would turn off the check for some cases we don't use (e.g., trying to restore output of pg_dump with binary upgrade set).

Test Plan:
Verify preserving all pg_class OIDs works for new version:
```
ybd --cxx-test yb-backup-cross-feature-test --gtest_filter '*.TestPreservingPgEnumOids' --test-args '--verbose_yb_backup' >& /tmp/generic.mdl.log
ybd --cxx-test yb-backup-cross-feature-test --gtest_filter '*.TestPreservingPgTypeAndClassOids' --test-args '--verbose_yb_backup' >& /tmp/generic.mdl.log
```

Other tests:
```
ybd  --java-test 'org.yb.pgsql.TestYsqlDump#ysqlDumpAllWithYbMetadata' >& /tmp/generic.mdl.log
ybd  --java-test 'org.yb.pgsql.TestYsqlDump#ysqlDumpWithYbMetadata' >& /tmp/generic.mdl.log2
ybd  --java-test 'org.yb.pgsql.TestYsqlDump#ysqlDumpColocatedDB' >& /tmp/generic.mdl.log3
ybd  --java-test 'org.yb.pgsql.TestYsqlDump#ysqlDumpLegacyColocatedDB' >& /tmp/generic.mdl.log4
ybd  --java-test 'org.yb.pgsql.TestYsqlDump#ysqlDumpWithoutYbMetadata' >& /tmp/generic.mdl.log5
```

I did not verify that backwards compatibility was maintained -- being able to import a PG 11 dump into PG 15 will likely require more than just this diff.

Reviewers: jason, tfoucher

Reviewed By: jason

Differential Revision: https://phorge.dev.yugabyte.com/D37810
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ysql Yugabyte SQL (YSQL) kind/new-feature This is a request for a completely new feature priority/medium Medium priority issue
Projects
None yet
Development

No branches or pull requests

2 participants