On Mon, 6 Nov 2023 at 07:51, Peter Smith <smithpb2...@gmail.com> wrote: > > Here are some review comments for patch v11-0001 > > ====== > Commit message > > 1. > The subscription's replication origin are needed to ensure > that we don't replicate anything twice. > > ~ > > /are needed/is needed/
Modified > > 2. > Author: Julien Rouhaud > Reviewed-by: FIXME > Discussion: https://postgr.es/m/20230217075433.u5mjly4d5cr4hcfe@jrouhaud > > ~ > > Include Vignesh as another author. Modified > ====== > doc/src/sgml/ref/pgupgrade.sgml > > 3. > + <application>pg_upgrade</application> attempts to migrate subscription > + dependencies which includes the subscription tables information present > in > + <link linkend="catalog-pg-subscription-rel">pg_subscription_rel</link> > + system table and the subscription replication origin which > + will help in continuing logical replication from where the old > subscriber > + was replicating. This helps in avoiding the need for setting up the > > I became a bit lost reading paragraph due to the multiple 'which'... > > SUGGESTION > pg_upgrade attempts to migrate subscription dependencies which > includes the subscription table information present in > pg_subscription_rel system > catalog and also the subscription replication origin. This allows > logical replication on the new subscriber to continue from where the > old subscriber was up to. Modified > ~~~ > > 4. > + was replicating. This helps in avoiding the need for setting up the > + subscription objects manually which requires truncating all the > + subscription tables and setting the logical replication slots. Migration > > SUGGESTION > Having the ability to migrate subscription objects avoids the need to > set them up manually, which would require truncating all the > subscription tables and setting the logical replication slots. I have removed this > ~ > > TBH, I am wondering what is the purpose of this sentence. It seems > more like a justification for the patch, but does the user need to > know all this? > > ~~~ > > 5. > + <para> > + All the subscription tables in the old subscriber should be in > + <literal>i</literal> (initialize), <literal>r</literal> (ready) or > + <literal>s</literal> (synchronized). This can be verified by checking > + <link > linkend="catalog-pg-subscription-rel">pg_subscription_rel</link>.<structfield>srsubstate</structfield>. > + </para> > > /should be in/should be in state/ Modified > ~~~ > > 6. > + <para> > + The replication origin entry corresponding to each of the > subscriptions > + should exist in the old cluster. This can be checking > + <link linkend="catalog-pg-subscription">pg_subscription</link> and > + <link > linkend="catalog-pg-replication-origin">pg_replication_origin</link> > + system tables. > + </para> > > missing words? > > /This can be checking/This can be found by checking/ Modified > ~~~ > > 7. > + <para> > + The subscriptions will be migrated to new cluster in disabled state, > they > + can be enabled after upgrade by following the steps: > + </para> > > The first bullet also says "Enable the subscription..." so I think > this paragraph should be worded like the below. > > SUGGESTION > The subscriptions will be migrated to the new cluster in a disabled > state. After migration, do this: Modified > ====== > src/backend/catalog/pg_subscription.c > > 8. > #include "nodes/makefuncs.h" > +#include "replication/origin.h" > +#include "replication/worker_internal.h" > #include "storage/lmgr.h" > > Why does this change need to be in the patch when there are no other > code changes in this file? Modified > ====== > src/backend/utils/adt/pg_upgrade_support.c > > 9. binary_upgrade_create_sub_rel_state > > IMO a better name for this function would be > 'binary_upgrade_add_sub_rel_state' (because it delegates to > AddSubscriptionRelState). > > Then it would obey the same name pattern as the other function > 'binary_upgrade_replorigin_advance' (which delegates to > replorigin_advance). Modified > ~~~ > > 10. > +/* > + * binary_upgrade_create_sub_rel_state > + * > + * Add the relation with the specified relation state to pg_subscription_rel > + * table. > + */ > +Datum > +binary_upgrade_create_sub_rel_state(PG_FUNCTION_ARGS) > +{ > + Relation rel; > + HeapTuple tup; > + Oid subid; > + Form_pg_subscription form; > + char *subname; > + Oid relid; > + char relstate; > + XLogRecPtr sublsn; > > 10a. > /to pg_subscription_rel table./to pg_subscription_rel catalog./ Modified > ~ > > 10b. > Maybe it would be helpful if the function argument were documented > up-front in the function-comment, or in the variable declarations. > > SUGGESTION > char *subname; /* ARG0 = subscription name */ > Oid relid; /* ARG1 = relation Oid */ > char relstate; /* ARG2 = subrel state */ > XLogRecPtr sublsn; /* ARG3 (optional) = subscription lsn */ I felt the variables are self explainatory in this case and also consistent with other functions. > ~~~ > > 11. > if (PG_ARGISNULL(3)) > sublsn = InvalidXLogRecPtr; > else > sublsn = PG_GETARG_LSN(3); > FWIW, I'd write that as a one-line ternary assignment allowing all the > args to be grouped nicely together. > > SUGGESTION > sublsn = PG_ARGISNULL(3) ? InvalidXLogRecPtr : PG_GETARG_LSN(3); Modified > ~~~ > > 12. binary_upgrade_replorigin_advance > > /* > * binary_upgrade_replorigin_advance > * > * Update the remote_lsn for the subscriber's replication origin. > */ > Datum > binary_upgrade_replorigin_advance(PG_FUNCTION_ARGS) > { > Relation rel; > HeapTuple tup; > Oid subid; > Form_pg_subscription form; > char *subname; > XLogRecPtr sublsn; > char originname[NAMEDATALEN]; > RepOriginId originid; > ~ > > Similar to previous comment #10b. Maybe it would be helpful if the > function argument were documented up-front in the function-comment, or > in the variable declarations. > > SUGGESTION > char originname[NAMEDATALEN]; > RepOriginId originid; > char *subname; /* ARG0 = subscription name */ > XLogRecPtr sublsn; /* ARG1 = subscription lsn */ I felt the variables are self explainatory in this case and also consistent with other functions. > ~~~ > > 13. > + subname = text_to_cstring(PG_GETARG_TEXT_PP(0)); > + > + if (PG_ARGISNULL(1)) > + sublsn = InvalidXLogRecPtr; > + else > + sublsn = PG_GETARG_LSN(1); > > Similar to previous comment #11. FWIW, I'd write that as a one-line > ternary assignment allowing all the args to be grouped nicely > together. > > SUGGESTION > subname = text_to_cstring(PG_GETARG_TEXT_PP(0)); > sublsn = PG_ARGISNULL(1) ? InvalidXLogRecPtr : PG_GETARG_LSN(1); Modified > ====== > src/bin/pg_dump/pg_dump.c > > 14. getSubscriptionTables > > +/* > + * getSubscriptionTables > + * get information about subscription membership for dumpable tables, this > + * will be used only in binary-upgrade mode. > + */ > > Should use multiple sentences. > > SUGGESTION > Get information about subscription membership for dumpable tables. > This will be used only in binary-upgrade mode. Modified > ~~~ > > 15. > + /* Get subscription relation fields */ > + i_srsubid = PQfnumber(res, "srsubid"); > + i_srrelid = PQfnumber(res, "srrelid"); > + i_srsubstate = PQfnumber(res, "srsubstate"); > + i_srsublsn = PQfnumber(res, "srsublsn"); > > Might it be better to say "Get pg_subscription_rel attributes"? Modified > ~~~ > > 16. getSubscriptions > > + appendPQExpBufferStr(query, "o.remote_lsn\n"); > appendPQExpBufferStr(query, > "FROM pg_subscription s\n" > + "LEFT JOIN pg_replication_origin_status o \n" > + " ON o.external_id = 'pg_' || s.oid::text \n" > "WHERE s.subdbid = (SELECT oid FROM pg_database\n" > " WHERE datname = current_database())"); > > ~ > > 16a. > Should that "remote_lsn" have an alias like "suboriginremotelsn" so > that it matches the later field assignment better? Modified > ~ > > 16b. > Probably these catalogs should be qualified using "pg_catalog.". Modified > ~~~ > > 17. dumpSubscriptionTable > > +/* > + * dumpSubscriptionTable > + * dump the definition of the given subscription table mapping, this will > be > + * used only for upgrade operation. > + */ > > Make this comment consistent with the other one for getSubscriptionTables: > - split into multiple sentences > - use the same terminology "binary-upgrade mode" versus "upgrade operation'. Modified > ~~~ > > 18. > + /* > + * binary_upgrade_create_sub_rel_state will add the subscription > + * relation to pg_subscripion_rel table, this is supported only for > + * upgrade operation. > + */ > > Split into multiple sentences. Modified > ====== > src/bin/pg_dump/pg_dump_sort.c > > 19. > + case DO_SUBSCRIPTION_REL: > + snprintf(buf, bufsize, > + "SUBSCRIPTION TABLE (ID %d)", > + obj->dumpId); > + return; > > Should it include the OID (like for DO PUBLICATION_TABLE)? Modified > ====== > src/bin/pg_upgrade/check.c > > 20. > check_for_reg_data_type_usage(&old_cluster); > check_for_isn_and_int8_passing_mismatch(&old_cluster); > > + check_for_subscription_state(&old_cluster); > + > > There seems no reason anymore for this check to be separated from all > the other checks. Just remove the blank line. Modified > ~~~ > > 21. check_for_subscription_state > > +/* > + * check_for_subscription_state() > + * > + * Verify that each of the subscriptions have all their corresponding tables > in > + * ready state. > + */ > +static void > +check_for_subscription_state(ClusterInfo *cluster) > > /have/has/ > > This comment only refers to 'ready' state, but perhaps it is > misleading (or not entirely correct) because later the SQL is testing > for more than just the READY state: > > + "WHERE srsubstate NOT IN ('i', 's', 'r') " Modified > ~~~ > > 22. > + res = executeQueryOrDie(conn, > + "SELECT s.subname, c.relname, n.nspname " > + "FROM pg_catalog.pg_subscription_rel r " > + "LEFT JOIN pg_catalog.pg_subscription s" > + " ON r.srsubid = s.oid " > + "LEFT JOIN pg_catalog.pg_class c" > + " ON r.srrelid = c.oid " > + "LEFT JOIN pg_catalog.pg_namespace n" > + " ON c.relnamespace = n.oid " > + "WHERE srsubstate NOT IN ('i', 's', 'r') " > + "ORDER BY s.subname"); > > If you are going to check 'i', 's', and 'r' then I thought this > statement should maybe have some comment about why those states. Modified > ~~~ > > 23. > + pg_fatal("Your installation contains subscription(s) with\n" > + "Subscription not having origin and/or subscription relation(s) not > in ready state.\n" > + "A list of subscription not having origin and/or\n" > + "subscription relation(s) not in ready state is in the file: %s", > + output_path); > > 23a. > This message seems to just be saying the same thing 2 times. > > Is also should use newlines and spaces more like the other similar > pg_patals in this file (e.g. the %s is on next line etc). > > SUGGESTION > Your installation contains subscriptions without origin or having > relations not in a ready state.\n > A list of the problem subscriptions is in the file:\n > %s Modified > ~ > > 23b. > Same question about 'not in ready state'. Is that entirely correct? Modified > ====== > src/bin/pg_upgrade/t/004_subscription.pl > > 24. > +sub insert_line > +{ > + my $payload = shift; > + > + foreach ("t1", "t2") > + { > + $publisher->safe_psql('postgres', > + "INSERT INTO " . $_ . " (val) VALUES('$payload')"); > + } > +} > > For clarity, maybe call this function 'insert_line_at_pub' Modified > ~~~ > > 25. > +# ------------------------------------------------------ > +# Check that pg_upgrade is succesful when all tables are in ready state. > +# ------------------------------------------------------ > > /succesful/successful/ Modified > ~~~ > > 26. > +command_ok( > + [ > + 'pg_upgrade', '--no-sync', '-d', $old_sub->data_dir, > + '-D', $new_sub->data_dir, '-b', $bindir, > + '-B', $bindir, '-s', $new_sub->host, > + '-p', $old_sub->port, '-P', $new_sub->port, > + $mode, '--check', > + ], > + 'run of pg_upgrade --check for old instance with invalid remote_lsn'); > > This is the command for the "success" case. Why is the message part > referring to "invalid remote_lsn"? Modified > ~~~ > > 27. > +$publisher->safe_psql('postgres', > + "CREATE TABLE tab_primary_key(id serial, val text);"); > +$old_sub->safe_psql('postgres', > + "CREATE TABLE tab_primary_key(id serial PRIMARY KEY, val text);"); > +$publisher->safe_psql('postgres', > > > Maybe it is not necessary, but won't it be better if the publisher > table also has a primary key (so DDL matches its table name)? Modified > ~~~ > > 28. > +# Add a row in subscriber so that the table sync will fail. > +$old_sub->safe_psql('postgres', > + "INSERT INTO tab_primary_key values(1, 'before initial sync')"); > > The comment should be slightly more descriptive by saying the reason > it will fail is that you deliberately inserted the same PK value > again. Modified > ~~~ > > 29. > +my $started_query = > + "SELECT count(1) = 1 FROM pg_subscription_rel WHERE srsubstate = 'd';"; > +$old_sub->poll_query_until('postgres', $started_query) > + or die "Timed out while waiting for subscriber to synchronize data"; > > Since this cannot synchronize the table data, maybe the message should > be more like "Timed out while waiting for the table state to become > 'd' (datasync)" Modified > ~~~ > > 30. > +command_fails( > + [ > + 'pg_upgrade', '--no-sync', '-d', $old_sub->data_dir, > + '-D', $new_sub->data_dir, '-b', $bindir, > + '-B', $bindir, '-s', $new_sub->host, > + '-p', $old_sub->port, '-P', $new_sub->port, > + $mode, '--check', > + ], > + 'run of pg_upgrade --check for old instance with incorrect sub rel'); > > /with incorrect sub rel/with incorrect sub rel state/ (??) Modified > ~~~ > > 31. > +# ------------------------------------------------------ > +# Check that pg_upgrade doesn't detect any problem once all the > subscription's > +# relation are in 'r' (ready) state. > +# ------------------------------------------------------ > > > 31a. > /relation/relations/ > I have removed this comment > > 31b. > Do you think that comment is correct? All you are doing here is > allowing the old_sub to proceed because there is no longer any > conflict -- but isn't that just normal pub/sub behaviour that has > nothing to do with pg_upgrade? I have removed this comment > ~~~ > > 32. > +# Stop the old subscriber, insert a row in each table while it's down and add > +# t2 to the publication > > /in each table/in each publisher table/ > > Also, it is not each table -- it's only t1 and t2; not tab_primary_key. Modified > ~~~ > > 33. > + $new_sub->safe_psql('postgres', "SELECT count(*) FROM > pg_subscription_rel"); > +is($result, qq(2), "There should be 2 rows in pg_subscription_rel"); > > /2 rows in pg_subscription_rel/2 rows in pg_subscription_rel > (representing t1 and tab_primary_key)/ Modified > ====== > > 34. binary_upgrade_create_sub_rel_state > > +{ oid => '8404', descr => 'for use by pg_upgrade (relation for > pg_subscription_rel)', > + proname => 'binary_upgrade_create_sub_rel_state', proisstrict => 'f', > + provolatile => 'v', proparallel => 'u', prorettype => 'void', > + proargtypes => 'text oid char pg_lsn', > + prosrc => 'binary_upgrade_create_sub_rel_state' }, > > As mentioned in a previous review comment #9, I felt this function > should have a different name: binary_upgrade_add_sub_rel_state. Modified Thanks for the comments, the attached v12 version patch has the changes for the same. Regards, Vignesh
From 50f1a2b3bac25d5bd709bacfc7e71c15e708776f Mon Sep 17 00:00:00 2001 From: Vignesh C <vignes...@gmail.com> Date: Mon, 30 Oct 2023 12:31:59 +0530 Subject: [PATCH v12] Preserve the full subscription's state during pg_upgrade Previously, only the subscription metadata information was preserved. Without the list of relations and their state it's impossible to re-enable the subscriptions without missing some records as the list of relations can only be refreshed after enabling the subscription (and therefore starting the apply worker). Even if we added a way to refresh the subscription while enabling a publication, we still wouldn't know which relations are new on the publication side, and therefore should be fully synced, and which shouldn't. To fix this problem, this patch teaches pg_dump to restore the content of pg_subscription_rel from the old cluster by using binary_upgrade_create_sub_rel_state SQL function. This is supported only in binary upgrade mode. The new SQL binary_upgrade_create_sub_rel_state function has the following syntax: SELECT binary_upgrade_create_sub_rel_state(subname text, relid oid, state char [,sublsn pg_lsn]) In the above, subname is the subscription name, relid is the relation identifier, the state is the state of the relation, sublsn is subscription lsn which is optional, and defaults to NULL/InvalidXLogRecPtr if not provided. pg_dump will retrieve these values(subname, relid, state and sublsn) from the old cluster. The subscription's replication origin is needed to ensure that we don't replicate anything twice. To fix this problem, this patch teaches pg_dump to update the replication origin along with create subscription by using binary_upgrade_replorigin_advance SQL function to restore the underlying replication origin remote LSN. This is supported only in binary upgrade mode. The new SQL binary_upgrade_replorigin_advance function has the following syntax: SELECT binary_upgrade_replorigin_advance(subname text, sublsn pg_lsn) In the above, subname is the subscription name and sublsn is subscription lsn. pg_dump will retrieve these values(subname and sublsn) from the old cluster. pg_upgrade will check that all the subscription relations are in 'i' (init), 's' (data sync) or in 'r' (ready) state, and will error out if that's not the case, logging the reason for the failure. Author: Julien Rouhaud, Vignesh C Reviewed-by: FIXME Discussion: https://postgr.es/m/20230217075433.u5mjly4d5cr4hcfe@jrouhaud --- doc/src/sgml/ref/pgupgrade.sgml | 64 +++++++ src/backend/utils/adt/pg_upgrade_support.c | 118 ++++++++++++ src/bin/pg_dump/common.c | 22 +++ src/bin/pg_dump/pg_dump.c | 201 ++++++++++++++++++- src/bin/pg_dump/pg_dump.h | 16 ++ src/bin/pg_dump/pg_dump_sort.c | 11 +- src/bin/pg_upgrade/check.c | 123 ++++++++++++ src/bin/pg_upgrade/meson.build | 1 + src/bin/pg_upgrade/t/004_subscription.pl | 213 +++++++++++++++++++++ src/include/catalog/pg_proc.dat | 10 + src/tools/pgindent/typedefs.list | 1 + 11 files changed, 774 insertions(+), 6 deletions(-) create mode 100644 src/bin/pg_upgrade/t/004_subscription.pl diff --git a/doc/src/sgml/ref/pgupgrade.sgml b/doc/src/sgml/ref/pgupgrade.sgml index 46e8a0b746..1a6ad16060 100644 --- a/doc/src/sgml/ref/pgupgrade.sgml +++ b/doc/src/sgml/ref/pgupgrade.sgml @@ -456,6 +456,70 @@ make prefix=/usr/local/pgsql.new install </step> + <step> + <title>Prepare for subscriber upgrades</title> + + <para> + Setup the <link linkend="logical-replication-config-subscriber"> + subscriber configurations</link> in the new subscriber. + <application>pg_upgrade</application> attempts to migrate subscription + dependencies which includes the subscription table information present in + <link linkend="catalog-pg-subscription-rel">pg_subscription_rel</link> + system catalog and also the subscription replication origin. This allows + logical replication on the new subscriber to continue from where the + old subscriber was up to. Migration of subscription dependencies is only + supported when the old cluster is version 17.0 or later. Subscription + dependencies on clusters before version 17.0 will silently be ignored. + </para> + + <para> + There are some prerequisites for <application>pg_upgrade</application> to + be able to upgrade the subscriptions. If these are not met an error + will be reported. + </para> + + <itemizedlist> + <listitem> + <para> + All the subscription tables in the old subscriber should be in state + <literal>i</literal> (initialize), <literal>r</literal> (ready) or + <literal>s</literal> (synchronized). This can be verified by checking + <link linkend="catalog-pg-subscription-rel">pg_subscription_rel</link>.<structfield>srsubstate</structfield>. + </para> + </listitem> + <listitem> + <para> + The replication origin entry corresponding to each of the subscriptions + should exist in the old cluster. This can be found by checking + <link linkend="catalog-pg-subscription">pg_subscription</link> and + <link linkend="catalog-pg-replication-origin">pg_replication_origin</link> + system tables. + </para> + </listitem> + </itemizedlist> + + <para> + The subscriptions will be migrated to the new cluster in a disabled state. + After migration, do this: + </para> + + <itemizedlist> + <listitem> + <para> + Enable the subscriptions by executing + <link linkend="sql-altersubscription"><command>ALTER SUBSCRIPTION ... ENABLE</command></link>. + </para> + </listitem> + <listitem> + <para> + Create all the new tables that were created in the publication during + upgrade and refresh the publication by executing + <link linkend="sql-altersubscription"><command>ALTER SUBSCRIPTION ... REFRESH PUBLICATION</command></link>. + </para> + </listitem> + </itemizedlist> + </step> + <step> <title>Stop both servers</title> diff --git a/src/backend/utils/adt/pg_upgrade_support.c b/src/backend/utils/adt/pg_upgrade_support.c index 2f6fc86c3d..4a3da80e49 100644 --- a/src/backend/utils/adt/pg_upgrade_support.c +++ b/src/backend/utils/adt/pg_upgrade_support.c @@ -11,15 +11,21 @@ #include "postgres.h" +#include "access/table.h" #include "catalog/binary_upgrade.h" #include "catalog/heap.h" #include "catalog/namespace.h" +#include "catalog/pg_subscription_rel.h" #include "catalog/pg_type.h" #include "commands/extension.h" #include "miscadmin.h" #include "replication/logical.h" +#include "replication/origin.h" +#include "replication/worker_internal.h" #include "utils/array.h" #include "utils/builtins.h" +#include "utils/pg_lsn.h" +#include "utils/syscache.h" #define CHECK_IS_BINARY_UPGRADE \ @@ -305,3 +311,115 @@ binary_upgrade_logical_slot_has_caught_up(PG_FUNCTION_ARGS) PG_RETURN_BOOL(!found_pending_wal); } + +/* + * binary_upgrade_add_sub_rel_state + * + * Add the relation with the specified relation state to pg_subscription_rel + * catalog. + */ +Datum +binary_upgrade_add_sub_rel_state(PG_FUNCTION_ARGS) +{ + Relation rel; + HeapTuple tup; + Oid subid; + Form_pg_subscription form; + char *subname; + Oid relid; + char relstate; + XLogRecPtr sublsn; + + CHECK_IS_BINARY_UPGRADE; + + /* We must check these things before dereferencing the arguments */ + if (PG_ARGISNULL(0) || PG_ARGISNULL(1) || PG_ARGISNULL(2)) + elog(ERROR, "null argument to binary_upgrade_add_sub_rel_state is not allowed"); + + subname = text_to_cstring(PG_GETARG_TEXT_PP(0)); + relid = PG_GETARG_OID(1); + relstate = PG_GETARG_CHAR(2); + sublsn = PG_ARGISNULL(3) ? InvalidXLogRecPtr : PG_GETARG_LSN(3); + + if (!OidIsValid(relid)) + ereport(ERROR, + errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("invalid relation identifier used: %u", relid)); + + tup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid)); + if (!HeapTupleIsValid(tup)) + ereport(ERROR, + errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("relation %u does not exist", relid)); + ReleaseSysCache(tup); + + rel = table_open(SubscriptionRelationId, RowExclusiveLock); + + /* Fetch the existing tuple. */ + tup = SearchSysCache2(SUBSCRIPTIONNAME, MyDatabaseId, + CStringGetDatum(subname)); + if (!HeapTupleIsValid(tup)) + ereport(ERROR, + errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("subscription \"%s\" does not exist", subname)); + + form = (Form_pg_subscription) GETSTRUCT(tup); + subid = form->oid; + + AddSubscriptionRelState(subid, relid, relstate, sublsn); + + ReleaseSysCache(tup); + table_close(rel, RowExclusiveLock); + + PG_RETURN_VOID(); +} + +/* + * binary_upgrade_replorigin_advance + * + * Update the remote_lsn for the subscriber's replication origin. + */ +Datum +binary_upgrade_replorigin_advance(PG_FUNCTION_ARGS) +{ + Relation rel; + HeapTuple tup; + Oid subid; + Form_pg_subscription form; + char *subname; + XLogRecPtr sublsn; + char originname[NAMEDATALEN]; + RepOriginId originid; + + CHECK_IS_BINARY_UPGRADE; + + /* We must check these things before dereferencing the arguments */ + if (PG_ARGISNULL(0)) + elog(ERROR, "null argument to binary_upgrade_replorigin_advance is not allowed"); + + subname = text_to_cstring(PG_GETARG_TEXT_PP(0)); + sublsn = PG_ARGISNULL(1) ? InvalidXLogRecPtr : PG_GETARG_LSN(1); + + rel = table_open(SubscriptionRelationId, RowExclusiveLock); + + /* Fetch the existing tuple. */ + tup = SearchSysCacheCopy2(SUBSCRIPTIONNAME, MyDatabaseId, + CStringGetDatum(subname)); + if (!HeapTupleIsValid(tup)) + ereport(ERROR, + errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("subscription \"%s\" does not exist", subname)); + + form = (Form_pg_subscription) GETSTRUCT(tup); + subid = form->oid; + + ReplicationOriginNameForLogicalRep(subid, InvalidOid, originname, sizeof(originname)); + originid = replorigin_by_name(originname, false); + replorigin_advance(originid, sublsn, InvalidXLogRecPtr, + false /* backward */ , + false /* WAL log */ ); + heap_freetuple(tup); + table_close(rel, RowExclusiveLock); + + PG_RETURN_VOID(); +} diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c index 8b0c1e7b53..764a39fcb9 100644 --- a/src/bin/pg_dump/common.c +++ b/src/bin/pg_dump/common.c @@ -24,6 +24,7 @@ #include "catalog/pg_operator_d.h" #include "catalog/pg_proc_d.h" #include "catalog/pg_publication_d.h" +#include "catalog/pg_subscription_d.h" #include "catalog/pg_type_d.h" #include "common/hashfn.h" #include "fe_utils/string_utils.h" @@ -265,6 +266,9 @@ getSchemaData(Archive *fout, int *numTablesPtr) pg_log_info("reading subscriptions"); getSubscriptions(fout); + pg_log_info("reading subscription membership of tables"); + getSubscriptionTables(fout); + free(inhinfo); /* not needed any longer */ *numTablesPtr = numTables; @@ -978,6 +982,24 @@ findPublicationByOid(Oid oid) return (PublicationInfo *) dobj; } +/* + * findSubscriptionByOid + * finds the DumpableObject for the subscription with the given oid + * returns NULL if not found + */ +SubscriptionInfo * +findSubscriptionByOid(Oid oid) +{ + CatalogId catId; + DumpableObject *dobj; + + catId.tableoid = SubscriptionRelationId; + catId.oid = oid; + dobj = findObjectByCatalogId(catId); + Assert(dobj == NULL || dobj->objType == DO_SUBSCRIPTION); + return (SubscriptionInfo *) dobj; +} + /* * recordExtensionMembership diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index e863913849..4e33493852 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -296,6 +296,7 @@ static void dumpPolicy(Archive *fout, const PolicyInfo *polinfo); static void dumpPublication(Archive *fout, const PublicationInfo *pubinfo); static void dumpPublicationTable(Archive *fout, const PublicationRelInfo *pubrinfo); static void dumpSubscription(Archive *fout, const SubscriptionInfo *subinfo); +static void dumpSubscriptionTable(Archive *fout, const SubRelInfo *subrinfo); static void dumpDatabase(Archive *fout); static void dumpDatabaseConfig(Archive *AH, PQExpBuffer outbuf, const char *dbname, Oid dboid); @@ -4581,6 +4582,99 @@ is_superuser(Archive *fout) return false; } +/* + * getSubscriptionTables + * Get information about subscription membership for dumpable tables. This + * will be used only in binary-upgrade mode. + */ +void +getSubscriptionTables(Archive *fout) +{ + DumpOptions *dopt = fout->dopt; + SubscriptionInfo *subinfo = NULL; + SubRelInfo *subrinfo; + PQExpBuffer query; + PGresult *res; + int i_srsubid; + int i_srrelid; + int i_srsubstate; + int i_srsublsn; + int i; + int cur_rel = 0; + int ntups; + Oid last_srsubid = InvalidOid; + + if (dopt->no_subscriptions || !dopt->binary_upgrade || + fout->remoteVersion < 170000) + return; + + query = createPQExpBuffer(); + appendPQExpBuffer(query, "SELECT srsubid, srrelid, srsubstate, srsublsn" + " FROM pg_catalog.pg_subscription_rel" + " ORDER BY srsubid"); + res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK); + + ntups = PQntuples(res); + if (ntups == 0) + goto cleanup; + + /* Get pg_subscription_rel attributes */ + i_srsubid = PQfnumber(res, "srsubid"); + i_srrelid = PQfnumber(res, "srrelid"); + i_srsubstate = PQfnumber(res, "srsubstate"); + i_srsublsn = PQfnumber(res, "srsublsn"); + + subrinfo = pg_malloc(ntups * sizeof(SubRelInfo)); + for (i = 0; i < ntups; i++) + { + Oid cur_srsubid = atooid(PQgetvalue(res, i, i_srsubid)); + Oid relid = atooid(PQgetvalue(res, i, i_srrelid)); + TableInfo *tblinfo; + + /* + * If we switched to a new subscription, check if the subscription + * exists. + */ + if (cur_srsubid != last_srsubid) + { + subinfo = findSubscriptionByOid(cur_srsubid); + if (subinfo == NULL) + pg_fatal("subscription with OID %u does not exist", cur_srsubid); + + last_srsubid = cur_srsubid; + } + + tblinfo = findTableByOid(relid); + if (tblinfo == NULL) + pg_fatal("failed sanity check, table with OID %u not found", + relid); + + /* OK, make a DumpableObject for this relationship */ + subrinfo[cur_rel].dobj.objType = DO_SUBSCRIPTION_REL; + subrinfo[cur_rel].dobj.catId.tableoid = relid; + subrinfo[cur_rel].dobj.catId.oid = cur_srsubid; + AssignDumpId(&subrinfo[cur_rel].dobj); + subrinfo[cur_rel].dobj.name = pg_strdup(subinfo->dobj.name); + subrinfo[cur_rel].tblinfo = tblinfo; + subrinfo[cur_rel].srsubstate = PQgetvalue(res, i, i_srsubstate)[0]; + if (PQgetisnull(res, i, i_srsublsn)) + subrinfo[cur_rel].srsublsn = NULL; + else + subrinfo[cur_rel].srsublsn = pg_strdup(PQgetvalue(res, i, i_srsublsn)); + + subrinfo[cur_rel].subinfo = subinfo; + + /* Decide whether we want to dump it */ + selectDumpableObject(&(subrinfo[cur_rel].dobj), fout); + + cur_rel++; + } + +cleanup: + PQclear(res); + destroyPQExpBuffer(query); +} + /* * getSubscriptions * get information about subscriptions @@ -4607,6 +4701,7 @@ getSubscriptions(Archive *fout) int i_subsynccommit; int i_subpublications; int i_suborigin; + int i_suboriginremotelsn; int i, ntups; @@ -4662,17 +4757,20 @@ getSubscriptions(Archive *fout) appendPQExpBufferStr(query, " s.subpasswordrequired,\n" " s.subrunasowner,\n" - " s.suborigin\n"); + " s.suborigin,\n"); else appendPQExpBuffer(query, " 't' AS subpasswordrequired,\n" " 't' AS subrunasowner,\n" - " '%s' AS suborigin\n", + " '%s' AS suborigin,\n", LOGICALREP_ORIGIN_ANY); + appendPQExpBufferStr(query, "o.remote_lsn AS suboriginremotelsn\n"); appendPQExpBufferStr(query, - "FROM pg_subscription s\n" - "WHERE s.subdbid = (SELECT oid FROM pg_database\n" + "FROM pg_catalog.pg_subscription s\n" + "LEFT JOIN pg_catalog.pg_replication_origin_status o \n" + " ON o.external_id = 'pg_' || s.oid::text \n" + "WHERE s.subdbid = (SELECT oid FROM pg_catalog.pg_database\n" " WHERE datname = current_database())"); res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK); @@ -4698,6 +4796,7 @@ getSubscriptions(Archive *fout) i_subsynccommit = PQfnumber(res, "subsynccommit"); i_subpublications = PQfnumber(res, "subpublications"); i_suborigin = PQfnumber(res, "suborigin"); + i_suboriginremotelsn = PQfnumber(res, "suboriginremotelsn"); subinfo = pg_malloc(ntups * sizeof(SubscriptionInfo)); @@ -4735,6 +4834,11 @@ getSubscriptions(Archive *fout) subinfo[i].subpublications = pg_strdup(PQgetvalue(res, i, i_subpublications)); subinfo[i].suborigin = pg_strdup(PQgetvalue(res, i, i_suborigin)); + if (PQgetisnull(res, i, i_suboriginremotelsn)) + subinfo[i].suboriginremotelsn = NULL; + else + subinfo[i].suboriginremotelsn = + pg_strdup(PQgetvalue(res, i, i_suboriginremotelsn)); /* Decide whether we want to dump it */ selectDumpableObject(&(subinfo[i].dobj), fout); @@ -4744,6 +4848,80 @@ getSubscriptions(Archive *fout) destroyPQExpBuffer(query); } +/* + * dumpSubscriptionTable + * Dump the definition of the given subscription table mapping. This will be + * used only in binary-upgrade mode. + */ +static void +dumpSubscriptionTable(Archive *fout, const SubRelInfo *subrinfo) +{ + DumpOptions *dopt = fout->dopt; + SubscriptionInfo *subinfo = subrinfo->subinfo; + PQExpBuffer query; + char *tag; + + /* Do nothing in data-only dump */ + if (dopt->dataOnly) + return; + + Assert(fout->dopt->binary_upgrade); + + tag = psprintf("%s %s", subinfo->dobj.name, subrinfo->dobj.name); + + query = createPQExpBuffer(); + + if (subinfo->dobj.dump & DUMP_COMPONENT_DEFINITION) + { + /* + * binary_upgrade_add_sub_rel_state will add the subscription + * relation to pg_subscripion_rel table. This will be used only in + * binary-upgrade mode. + */ + if (fout->remoteVersion >= 170000) + { + appendPQExpBufferStr(query, + "\n-- For binary upgrade, must preserve the subscriber table.\n"); + appendPQExpBufferStr(query, + "SELECT pg_catalog.binary_upgrade_add_sub_rel_state("); + appendStringLiteralAH(query, subrinfo->dobj.name, fout); + appendPQExpBuffer(query, + ", %u, '%c'", + subrinfo->tblinfo->dobj.catId.oid, + subrinfo->srsubstate); + + if (subrinfo->srsublsn && subrinfo->srsublsn[0] != '\0') + appendPQExpBuffer(query, ", '%s'", + subrinfo->srsublsn); + else + appendPQExpBuffer(query, ", NULL"); + + appendPQExpBufferStr(query, ");\n"); + } + } + + /* + * There is no point in creating a drop query as the drop is done by table + * drop. (If you think to change this, see also _printTocEntry().) + * Although this object doesn't really have ownership as such, set the + * owner field anyway to ensure that the command is run by the correct + * role at restore time. + */ + if (subrinfo->dobj.dump & DUMP_COMPONENT_DEFINITION) + ArchiveEntry(fout, subrinfo->dobj.catId, subrinfo->dobj.dumpId, + ARCHIVE_OPTS(.tag = tag, + .namespace = subrinfo->tblinfo->dobj.namespace->dobj.name, + .owner = subinfo->rolname, + .description = "SUBSCRIPTION TABLE", + .section = SECTION_POST_DATA, + .createStmt = query->data)); + + /* These objects can't currently have comments or seclabels */ + + free(tag); + destroyPQExpBuffer(query); +} + /* * dumpSubscription * dump the definition of the given subscription @@ -4824,6 +5002,17 @@ dumpSubscription(Archive *fout, const SubscriptionInfo *subinfo) appendPQExpBufferStr(query, ");\n"); + if (dopt->binary_upgrade && fout->remoteVersion >= 170000 && + subinfo->suboriginremotelsn) + { + appendPQExpBufferStr(query, + "\n-- For binary upgrade, must preserve the remote_lsn for the subscriber's replication origin.\n"); + appendPQExpBufferStr(query, + "SELECT pg_catalog.binary_upgrade_replorigin_advance("); + appendStringLiteralAH(query, subinfo->dobj.name, fout); + appendPQExpBuffer(query, ", '%s');\n", subinfo->suboriginremotelsn); + } + if (subinfo->dobj.dump & DUMP_COMPONENT_DEFINITION) ArchiveEntry(fout, subinfo->dobj.catId, subinfo->dobj.dumpId, ARCHIVE_OPTS(.tag = subinfo->dobj.name, @@ -10442,6 +10631,9 @@ dumpDumpableObject(Archive *fout, DumpableObject *dobj) case DO_SUBSCRIPTION: dumpSubscription(fout, (const SubscriptionInfo *) dobj); break; + case DO_SUBSCRIPTION_REL: + dumpSubscriptionTable(fout, (const SubRelInfo *) dobj); + break; case DO_PRE_DATA_BOUNDARY: case DO_POST_DATA_BOUNDARY: /* never dumped, nothing to do */ @@ -18508,6 +18700,7 @@ addBoundaryDependencies(DumpableObject **dobjs, int numObjs, case DO_PUBLICATION_REL: case DO_PUBLICATION_TABLE_IN_SCHEMA: case DO_SUBSCRIPTION: + case DO_SUBSCRIPTION_REL: /* Post-data objects: must come after the post-data boundary */ addObjectDependency(dobj, postDataBound->dumpId); break; diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h index 2fe3cbed9a..62b3d9249b 100644 --- a/src/bin/pg_dump/pg_dump.h +++ b/src/bin/pg_dump/pg_dump.h @@ -83,6 +83,7 @@ typedef enum DO_PUBLICATION_REL, DO_PUBLICATION_TABLE_IN_SCHEMA, DO_SUBSCRIPTION, + DO_SUBSCRIPTION_REL, } DumpableObjectType; /* @@ -671,8 +672,21 @@ typedef struct _SubscriptionInfo char *subsynccommit; char *subpublications; char *suborigin; + char *suboriginremotelsn; } SubscriptionInfo; +/* + * The SubRelInfo struct is used to represent a subscription relation. + */ +typedef struct _SubRelInfo +{ + DumpableObject dobj; + SubscriptionInfo *subinfo; + TableInfo *tblinfo; + char srsubstate; + char *srsublsn; +} SubRelInfo; + /* * common utility functions */ @@ -697,6 +711,7 @@ extern CollInfo *findCollationByOid(Oid oid); extern NamespaceInfo *findNamespaceByOid(Oid oid); extern ExtensionInfo *findExtensionByOid(Oid oid); extern PublicationInfo *findPublicationByOid(Oid oid); +extern SubscriptionInfo *findSubscriptionByOid(Oid oid); extern void recordExtensionMembership(CatalogId catId, ExtensionInfo *ext); extern ExtensionInfo *findOwningExtension(CatalogId catalogId); @@ -756,5 +771,6 @@ extern void getPublicationNamespaces(Archive *fout); extern void getPublicationTables(Archive *fout, TableInfo tblinfo[], int numTables); extern void getSubscriptions(Archive *fout); +extern void getSubscriptionTables(Archive *fout); #endif /* PG_DUMP_H */ diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c index abfea15c09..e8d9c8ac86 100644 --- a/src/bin/pg_dump/pg_dump_sort.c +++ b/src/bin/pg_dump/pg_dump_sort.c @@ -94,6 +94,7 @@ enum dbObjectTypePriorities PRIO_PUBLICATION_REL, PRIO_PUBLICATION_TABLE_IN_SCHEMA, PRIO_SUBSCRIPTION, + PRIO_SUBSCRIPTION_REL, PRIO_DEFAULT_ACL, /* done in ACL pass */ PRIO_EVENT_TRIGGER, /* must be next to last! */ PRIO_REFRESH_MATVIEW /* must be last! */ @@ -147,10 +148,11 @@ static const int dbObjectTypePriority[] = PRIO_PUBLICATION, /* DO_PUBLICATION */ PRIO_PUBLICATION_REL, /* DO_PUBLICATION_REL */ PRIO_PUBLICATION_TABLE_IN_SCHEMA, /* DO_PUBLICATION_TABLE_IN_SCHEMA */ - PRIO_SUBSCRIPTION /* DO_SUBSCRIPTION */ + PRIO_SUBSCRIPTION, /* DO_SUBSCRIPTION */ + PRIO_SUBSCRIPTION_REL /* DO_SUBSCRIPTION_REL */ }; -StaticAssertDecl(lengthof(dbObjectTypePriority) == (DO_SUBSCRIPTION + 1), +StaticAssertDecl(lengthof(dbObjectTypePriority) == (DO_SUBSCRIPTION_REL + 1), "array length mismatch"); static DumpId preDataBoundId; @@ -1472,6 +1474,11 @@ describeDumpableObject(DumpableObject *obj, char *buf, int bufsize) "SUBSCRIPTION (ID %d OID %u)", obj->dumpId, obj->catId.oid); return; + case DO_SUBSCRIPTION_REL: + snprintf(buf, bufsize, + "SUBSCRIPTION TABLE (ID %d OID %u)", + obj->dumpId, obj->catId.oid); + return; case DO_PRE_DATA_BOUNDARY: snprintf(buf, bufsize, "PRE-DATA BOUNDARY (ID %d)", diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c index fa52aa2c22..70d3087e9f 100644 --- a/src/bin/pg_upgrade/check.c +++ b/src/bin/pg_upgrade/check.c @@ -20,6 +20,7 @@ static void check_is_install_user(ClusterInfo *cluster); static void check_proper_datallowconn(ClusterInfo *cluster); static void check_for_prepared_transactions(ClusterInfo *cluster); static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster); +static void check_for_subscription_state(ClusterInfo *cluster); static void check_for_user_defined_postfix_ops(ClusterInfo *cluster); static void check_for_incompatible_polymorphics(ClusterInfo *cluster); static void check_for_tables_with_oids(ClusterInfo *cluster); @@ -111,6 +112,7 @@ check_and_dump_old_cluster(bool live_check) check_for_composite_data_type_usage(&old_cluster); check_for_reg_data_type_usage(&old_cluster); check_for_isn_and_int8_passing_mismatch(&old_cluster); + check_for_subscription_state(&old_cluster); /* * Logical replication slots can be migrated since PG17. See comments atop @@ -812,6 +814,127 @@ check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster) check_ok(); } +/* + * check_for_subscription_state() + * + * Verify that each of the subscriptions has all their corresponding tables in + * i (initialize), r (ready) or s (synchronized) state. + */ +static void +check_for_subscription_state(ClusterInfo *cluster) +{ + int dbnum; + FILE *script = NULL; + char output_path[MAXPGPATH]; + int ntup; + + /* Subscription relations state can be migrated since PG17. */ + if (GET_MAJOR_VERSION(old_cluster.major_version) < 1700) + return; + + prep_status("Checking for subscription state"); + + snprintf(output_path, sizeof(output_path), "%s/%s", + log_opts.basedir, + "subscription_state.txt"); + for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++) + { + PGresult *res; + DbInfo *active_db = &cluster->dbarr.dbs[dbnum]; + PGconn *conn = connectToServer(cluster, active_db->db_name); + + /* We need to check for pg_replication_origin only once. */ + if (dbnum == 0) + { + /* + * Check that all the subscriptions have their respective + * replication origin. + */ + res = executeQueryOrDie(conn, + "SELECT d.datname, s.subname " + "FROM pg_catalog.pg_subscription s " + "LEFT OUTER JOIN pg_catalog.pg_replication_origin o " + " ON o.roname = 'pg_' || s.oid " + "INNER JOIN pg_catalog.pg_database d " + " ON d.oid = s.subdbid " + "WHERE o.roname iS NULL;"); + + ntup = PQntuples(res); + for (int i = 0; i < ntup; i++) + { + if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL) + pg_fatal("could not open file \"%s\": %s", + output_path, strerror(errno)); + fprintf(script, "replication origin is missing for database:%s subscription:%s\n", + PQgetvalue(res, i, 0), + PQgetvalue(res, i, 1)); + } + PQclear(res); + } + + /* + * The subscription relation should be in either i (initialize), + * r (ready) or s (synchronized) state as either the replication slot + * is not created or the replication slot is already dropped and the + * required WAL files will be present in the publisher. The other + * states are not ok as the worker has dependency on the replication + * slot/origin in these case: + * a) SUBREL_STATE_DATASYNC: In this case, the table sync worker will + * try to drop the replication slot but as the replication slots will + * be created with old subscription id in the publisher and the + * upgraded subscriber will not be able to clean the slots in this + * case. + * b) SUBREL_STATE_FINISHEDCOPY: In this case, the tablesync worker will + * expect the origin to be already existing as the origin is created + * with an old subscription id, tablesync worker will not be able to + * find the origin in this case. + * c) SUBREL_STATE_SYNCWAIT, SUBREL_STATE_CATCHUP and + * SUBREL_STATE_UNKNOWN: These states are not stored in the catalog, + * so we need not allow these states. + */ + res = executeQueryOrDie(conn, + "SELECT s.subname, c.relname, n.nspname, r.srsubstate " + "FROM pg_catalog.pg_subscription_rel r " + "LEFT JOIN pg_catalog.pg_subscription s" + " ON r.srsubid = s.oid " + "LEFT JOIN pg_catalog.pg_class c" + " ON r.srrelid = c.oid " + "LEFT JOIN pg_catalog.pg_namespace n" + " ON c.relnamespace = n.oid " + "WHERE r.srsubstate NOT IN ('i', 'r', 's') " + "ORDER BY s.subname"); + + ntup = PQntuples(res); + for (int i = 0; i < ntup; i++) + { + if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL) + pg_fatal("could not open file \"%s\": %s", + output_path, strerror(errno)); + + fprintf(script, "database:%s subscription:%s schema:%s relation:%s state:%s not in required state\n", + active_db->db_name, + PQgetvalue(res, i, 0), + PQgetvalue(res, i, 1), + PQgetvalue(res, i, 2), + PQgetvalue(res, i, 3)); + } + + PQclear(res); + PQfinish(conn); + } + + if (script) + { + fclose(script); + pg_log(PG_REPORT, "fatal"); + pg_fatal("Your installation contains subscriptions without origin or having relations not in i (initialize), r (ready) or s (synchronized) state.\n" + "A list of problem subscriptions is in the file:\n" + " %s", output_path); + } + else + check_ok(); +} + /* * Verify that no user defined postfix operators exist. */ diff --git a/src/bin/pg_upgrade/meson.build b/src/bin/pg_upgrade/meson.build index 3e8a08e062..32f12f9e27 100644 --- a/src/bin/pg_upgrade/meson.build +++ b/src/bin/pg_upgrade/meson.build @@ -43,6 +43,7 @@ tests += { 't/001_basic.pl', 't/002_pg_upgrade.pl', 't/003_logical_slots.pl', + 't/004_subscription.pl', ], 'test_kwargs': {'priority': 40}, # pg_upgrade tests are slow }, diff --git a/src/bin/pg_upgrade/t/004_subscription.pl b/src/bin/pg_upgrade/t/004_subscription.pl new file mode 100644 index 0000000000..ee6029b9b7 --- /dev/null +++ b/src/bin/pg_upgrade/t/004_subscription.pl @@ -0,0 +1,213 @@ +# Copyright (c) 2023, PostgreSQL Global Development Group + +# Test for pg_upgrade of logical subscription +use strict; +use warnings; + +use File::Path qw(rmtree); + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +# Can be changed to test the other modes. +my $mode = $ENV{PG_TEST_PG_UPGRADE_MODE} || '--copy'; + +# Initialize publisher node +my $publisher = PostgreSQL::Test::Cluster->new('publisher'); +$publisher->init(allows_streaming => 'logical'); +$publisher->start; + +# Initialize the old subscriber node +my $old_sub = PostgreSQL::Test::Cluster->new('old_sub'); +$old_sub->init; +$old_sub->start; + +# Initialize the new subscriber +my $new_sub = PostgreSQL::Test::Cluster->new('new_sub'); +$new_sub->init; +my $bindir = $new_sub->config_data('--bindir'); + +sub insert_line_at_pub +{ + my $payload = shift; + + foreach ("t1", "t2") + { + $publisher->safe_psql('postgres', + "INSERT INTO " . $_ . " (val) VALUES('$payload')"); + } +} + +# Initial setup +foreach ("t1", "t2") +{ + $publisher->safe_psql('postgres', + "CREATE TABLE " . $_ . " (id serial, val text)"); + $old_sub->safe_psql('postgres', + "CREATE TABLE " . $_ . " (id serial, val text)"); +} +insert_line_at_pub('before initial sync'); + +# Setup logical replication, replicating only 1 table +my $connstr = $publisher->connstr . ' dbname=postgres'; + +$publisher->safe_psql('postgres', + "CREATE PUBLICATION regress_pub FOR TABLE t1"); + +$old_sub->safe_psql('postgres', + "CREATE SUBSCRIPTION regress_sub CONNECTION '$connstr' PUBLICATION regress_pub" +); + +# Wait for the catchup, as we need the subscription rel in ready state +$old_sub->wait_for_subscription_sync($publisher, 'regress_sub'); + +# ------------------------------------------------------ +# Check that pg_upgrade is successful when all tables are in ready state. +# ------------------------------------------------------ +my $synced_query = + "SELECT count(1) = 0 FROM pg_subscription_rel WHERE srsubstate NOT IN ('r');"; +$old_sub->poll_query_until('postgres', $synced_query) + or die "Timed out while waiting for subscriber to synchronize data"; + +command_ok( + [ + 'pg_upgrade', '--no-sync', '-d', $old_sub->data_dir, + '-D', $new_sub->data_dir, '-b', $bindir, + '-B', $bindir, '-s', $new_sub->host, + '-p', $old_sub->port, '-P', $new_sub->port, + $mode, '--check', + ], + 'run of pg_upgrade --check for old instance when the subscription tables are in ready state'); +ok( !-d $new_sub->data_dir . "/pg_upgrade_output.d", + "pg_upgrade_output.d/ removed after successful pg_upgrade"); + +# Check the number of rows for each table on each server +my $result = $publisher->safe_psql('postgres', "SELECT count(*) FROM t1"); +is($result, qq(1), "check initial t1 table data on publisher"); +$result = $publisher->safe_psql('postgres', "SELECT count(*) FROM t2"); +is($result, qq(1), "check initial t1 table data on publisher"); +$result = $old_sub->safe_psql('postgres', "SELECT count(*) FROM t1"); +is($result, qq(1), "check initial t1 table data on the old subscriber"); +$result = $old_sub->safe_psql('postgres', "SELECT count(*) FROM t2"); +is($result, qq(0), "check initial t2 table data on the old subscriber"); + +# ------------------------------------------------------ +# Check that pg_upgrade refuses to run if there's a subscription with tables in +# a state different than 'r' (ready), 'i' (init) and 's' (synchronized). +# ------------------------------------------------------ + +$publisher->safe_psql('postgres', + "CREATE TABLE tab_primary_key(id serial PRIMARY KEY, val text);"); +$old_sub->safe_psql('postgres', + "CREATE TABLE tab_primary_key(id serial PRIMARY KEY, val text);"); +$publisher->safe_psql('postgres', + "INSERT INTO tab_primary_key values(1, 'before initial sync')"); + +# Insert the same value that is already present in publisher to the primary key +# column of subscriber so that the table sync will fail. +$old_sub->safe_psql('postgres', + "INSERT INTO tab_primary_key values(1, 'before initial sync')"); + +$publisher->safe_psql('postgres', + "ALTER PUBLICATION regress_pub ADD TABLE tab_primary_key"); +$old_sub->safe_psql('postgres', + "ALTER SUBSCRIPTION regress_sub REFRESH PUBLICATION"); + +# Table will be in 'd' (data is being copied) state as table sync will fail +# because of primary key constraint error. +my $started_query = + "SELECT count(1) = 1 FROM pg_subscription_rel WHERE srsubstate = 'd';"; +$old_sub->poll_query_until('postgres', $started_query) + or die "Timed out while waiting for the table state to become 'd' (datasync)"; + +command_fails( + [ + 'pg_upgrade', '--no-sync', '-d', $old_sub->data_dir, + '-D', $new_sub->data_dir, '-b', $bindir, + '-B', $bindir, '-s', $new_sub->host, + '-p', $old_sub->port, '-P', $new_sub->port, + $mode, '--check', + ], + 'run of pg_upgrade --check for old instance with relation in \'d\' datasync(invalid) state'); +rmtree($new_sub->data_dir . "/pg_upgrade_output.d"); + +# Delete the table data so that the primary key violation error will not happen +# and tab_primary_key reaches ready state. +$old_sub->safe_psql('postgres', "DELETE FROM tab_primary_key"); + +$old_sub->poll_query_until('postgres', $synced_query) + or die "Timed out while waiting for subscriber to synchronize data"; + +# ------------------------------------------------------ +# The incremental changes added to the publisher are replicated after upgrade. +# ------------------------------------------------------ + +# Stop the old subscriber, insert a row in t1 and t2 publisher table while it's +# down and add t2 to the publication. +my $remote_lsn = $old_sub->safe_psql('postgres', + "SELECT remote_lsn FROM pg_replication_origin_status"); +$old_sub->stop; + +insert_line_at_pub('while old_sub is down'); + +# Run pg_upgrade +command_ok( + [ + 'pg_upgrade', '--no-sync', '-d', $old_sub->data_dir, + '-D', $new_sub->data_dir, '-b', $bindir, + '-B', $bindir, '-s', $new_sub->host, + '-p', $old_sub->port, '-P', $new_sub->port, + $mode, + ], + 'run of pg_upgrade for new sub'); +ok( !-d $new_sub->data_dir . "/pg_upgrade_output.d", + "pg_upgrade_output.d/ removed after pg_upgrade success"); +$publisher->safe_psql('postgres', + "ALTER PUBLICATION regress_pub ADD TABLE t2"); + +$new_sub->start; + +# Subscription relations and replication origin remote_lsn should be preserved +$result = + $new_sub->safe_psql('postgres', "SELECT count(*) FROM pg_subscription_rel"); +is($result, qq(2), "There should be 2 rows in pg_subscription_rel(representing t1 and tab_primary_key)"); + +$result = $new_sub->safe_psql('postgres', + "SELECT remote_lsn FROM pg_replication_origin_status os, pg_subscription s where os.external_id = 'pg_' || s.oid" +); +is($result, qq($remote_lsn), "remote_lsn should have been preserved"); + +# There should be no new replicated rows before enabling the subscription +$result = $new_sub->safe_psql('postgres', "SELECT count(*) FROM t1"); +is($result, qq(1), + "t1 table has no new replicated rows before enabling the subscription"); +$result = $new_sub->safe_psql('postgres', "SELECT count(*) FROM t2"); +is($result, qq(0), + "no change in t2 table which is not part of the publication"); + +# Enable the subscription +$new_sub->safe_psql('postgres', "ALTER SUBSCRIPTION regress_sub ENABLE"); + +$publisher->wait_for_catchup('regress_sub'); + +# Rows on t1 should have been replicated, while nothing should happen for t2 +$result = $new_sub->safe_psql('postgres', "SELECT count(*) FROM t1"); +is($result, qq(2), "check replicated inserts on new subscriber"); +$result = $new_sub->safe_psql('postgres', "SELECT count(*) FROM t2"); +is($result, qq(0), + "no change in table t2 afer enable subscription which is not part of the publication" +); + +# Refresh the subscription, only the missing row on t2 should be replicated +$new_sub->safe_psql('postgres', + "ALTER SUBSCRIPTION regress_sub REFRESH PUBLICATION"); +$new_sub->wait_for_subscription_sync($publisher, 'regress_sub'); +$result = $new_sub->safe_psql('postgres', "SELECT count(*) FROM t1"); +is($result, qq(2), + "check there is no change when there was no changes replicated"); +$result = $new_sub->safe_psql('postgres', "SELECT count(*) FROM t2"); +is($result, qq(2), + "check replicated inserts on new subscriber after refreshing"); + +done_testing(); diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index f14aed422a..c7bf3cbd55 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -11382,6 +11382,16 @@ provolatile => 'v', proparallel => 'u', prorettype => 'bool', proargtypes => 'name', prosrc => 'binary_upgrade_logical_slot_has_caught_up' }, +{ oid => '8404', descr => 'for use by pg_upgrade (relation for pg_subscription_rel)', + proname => 'binary_upgrade_add_sub_rel_state', proisstrict => 'f', + provolatile => 'v', proparallel => 'u', prorettype => 'void', + proargtypes => 'text oid char pg_lsn', + prosrc => 'binary_upgrade_add_sub_rel_state' }, +{ oid => '8405', descr => 'for use by pg_upgrade (remote_lsn for origin)', + proname => 'binary_upgrade_replorigin_advance', proisstrict => 'f', + provolatile => 'v', proparallel => 'u', prorettype => 'void', + proargtypes => 'text pg_lsn', + prosrc => 'binary_upgrade_replorigin_advance' }, # conversion functions { oid => '4302', diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 87c1aee379..90b321945c 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2656,6 +2656,7 @@ SubLinkType SubOpts SubPlan SubPlanState +SubRelInfo SubRemoveRels SubTransactionId SubXactCallback -- 2.34.1
