Re: speed up a logical replica setup

On Tue, Mar 19, 2024 at 8:54 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Mon, 18 Mar 2024 at 16:36, Peter Eisentraut <peter@eisentraut.org> wrote:
> >
> > On 18.03.24 08:18, vignesh C wrote:
> > > 1) Maximum size of the object name is 64, we can have a check so that
> > > we don't specify more than the maximum allowed length:
> > > + case 3:
> > > + if (!simple_string_list_member(&opt.replslot_names, optarg))
> > > + {
> > > + simple_string_list_append(&opt.replslot_names, optarg);
> > > + num_replslots++;
> > > + }
> > > + else
> > > + {
> > > + pg_log_error("duplicate replication slot \"%s\"", optarg);
> > > + exit(1);
> > > + }
> > > + break;
> > >
> > > If we allow something like this:
> > >   ./pg_createsubscriber -U postgres -D data_N2/ -P "port=5431
> > > user=postgres"  -p 5432 -s /home/vignesh/postgres/inst/bin/ -d db1 -d
> > > db2 -d db3 --replication-slot="testtesttesttesttesttesttesttesttesttesttesttesttesttesttesttes1"
> > > --replication-slot="testtesttesttesttesttesttesttesttesttesttesttesttesttesttesttes2"
> > > --replication-slot="testtesttesttesttesttesttesttesttesttesttesttesttesttesttesttes3"
> > > In this case creation of replication slot will fail:
> > > pg_createsubscriber: error: could not create replication slot
> > > "testtesttesttesttesttesttesttesttesttesttesttesttesttesttesttes" on
> > > database "db2": ERROR:  replication slot
> > > "testtesttesttesttesttesttesttesttesttesttesttesttesttesttesttes"
> > > already exists
> >
> > I think this is fine.  The server can check whether the names it is
> > given are of the right size.  We don't need to check it again in the client.
> >
> > > 2) Similarly here too:
> > > + case 4:
> > > + if (!simple_string_list_member(&opt.sub_names, optarg))
> > > + {
> > > + simple_string_list_append(&opt.sub_names, optarg);
> > > + num_subs++;
> > > + }
> > > + else
> > > + {
> > > + pg_log_error("duplicate subscription \"%s\"", optarg);
> > > + exit(1);
> > > + }
> > > + break;
> > >
> > > If we allow something like this:
> > > ./pg_createsubscriber -U postgres -D data_N2/ -P "port=5431
> > > user=postgres"  -p 5432 -s /home/vignesh/postgres/inst/bin/ -d db1 -d
> > > db2 -d db3 --subscription=testtesttesttesttesttesttesttesttesttesttesttesttesttesttesttes1
> > > --subscription=testtesttesttesttesttesttesttesttesttesttesttesttesttesttesttes2
> > > --subscription=testtesttesttesttesttesttesttesttesttesttesttesttesttesttesttes3
> > >
> > > Subscriptions will be created with the same name and later there will
> > > be a problem when setting replication progress as there will be
> > > multiple subscriptions with the same name.
> >
> > Could you clarify this problem?
>
> In this case the subscriptions name specified is more than the allowed
> name, the subscription name will be truncated and both the
> subscription for db1 and db2 will have same name like below:
> db2=# select subname, subdbid from pg_subscription;
>                              subname                             | subdbid
> -----------------------------------------------------------------+---------
>  testtesttesttesttesttesttesttesttesttesttesttesttesttesttesttes |   16384
>  testtesttesttesttesttesttesttesttesttesttesttesttesttesttesttes |   16385
>
> Now we try to set the replication origin of the subscriptions to a
> consistent lsn from the following:
> +set_replication_progress(PGconn *conn, struct LogicalRepInfo *dbinfo,
> const char *lsn)
> +{
> +       PQExpBuffer str = createPQExpBuffer();
> +       PGresult   *res;
> +       Oid                     suboid;
> +       char            originname[NAMEDATALEN];
> +       char            lsnstr[17 + 1]; /* MAXPG_LSNLEN = 17 */
> +
> +       Assert(conn != NULL);
> +
> +       appendPQExpBuffer(str,
> +                                         "SELECT oid FROM
> pg_catalog.pg_subscription "
> +                                         "WHERE subname = '%s'",
> +                                         dbinfo->subname);
> +
> +       res = PQexec(conn, str->data);
> +       if (PQresultStatus(res) != PGRES_TUPLES_OK)
> +       {
> +               pg_log_error("could not obtain subscription OID: %s",
> +                                        PQresultErrorMessage(res));
> +               disconnect_database(conn, true);
> +       }
> +
> +       if (PQntuples(res) != 1 && !dry_run)
> +       {
> +               pg_log_error("could not obtain subscription OID: got
> %d rows, expected %d rows",
> +                                        PQntuples(res), 1);
> +               disconnect_database(conn, true);
> +       }
>
> Since the subscription name is truncated, we will have multiple
> records returned for the above query which results in failure with:
> pg_createsubscriber: error: could not obtain subscription OID: got 2
> rows, expected 1 rows
> pg_createsubscriber: warning: pg_createsubscriber failed after the end
> of recovery
> pg_createsubscriber: hint: The target server cannot be used as a
> physical replica anymore.
> pg_createsubscriber: hint: You must recreate the physical replica
> before continuing.
>
> The problem with this failure is that standby has been promoted
> already and we will have to re-create the physica replica again.
>
> If you are not planning to have the checks for name length, this could
> alternatively be fixed by including database id also while querying
> pg_subscription like below in set_replication_progress function:
> appendPQExpBuffer(str,
> "SELECT oid FROM pg_catalog.pg_subscription \n"
> "WHERE subname = '%s' AND subdbid = (SELECT oid FROM
> pg_catalog.pg_database WHERE datname = '%s')",
> dbinfo->subname,
> dbinfo->dbname);

The attached patch has the changes to handle the same.

Thanks and Regards,
Shubham Khanna.