On Wed, Jun 26, 2024 at 7:21 AM Euler Taveira <eu...@eulerto.com> wrote:
>
> On Mon, Jun 24, 2024, at 3:47 AM, Hayato Kuroda (Fujitsu) wrote:
>
> > pg_createsubscriber fails on a dbname containing a space. Use
> > appendConnStrVal() here and for other params in get_sub_conninfo(). See the
> > CVE-2016-5424 commits for more background. For one way to test this
> > scenario,
> > see generate_db() in the pg_upgrade test suite.
>
> Thanks for pointing out. I made a fix patch. Test code was also modified
> accordingly.
>
>
> Patch looks good to me. I have one suggestion
>
> -# Mostly Ported from 002_pg_upgrade.pl, but this returns a generated dbname.
> +# Extracted from 002_pg_upgrade.pl.
>
> and 2 small fixes:
>
> - 'logical replication works on database $db1');
> + "logical replication works on database $db1");
>
> -is($result, qq(row 1), 'logical replication works on database $db2');
> +is($result, qq(row 1), "logical replication works on database $db2");
>
> > > +static char *
> > > +create_logical_replication_slot(PGconn *conn, struct LogicalRepInfo
> > > *dbinfo)
> > > +{
> > > + PQExpBuffer str = createPQExpBuffer();
> > > + PGresult *res = NULL;
> > > + const char *slot_name = dbinfo->replslotname;
> > > + char *slot_name_esc;
> > > + char *lsn = NULL;
> > > +
> > > + Assert(conn != NULL);
> > > +
> > > + pg_log_info("creating the replication slot \"%s\" on database \"%s\"",
> > > + slot_name, dbinfo->dbname);
> > > +
> > > + slot_name_esc = PQescapeLiteral(conn, slot_name, strlen(slot_name));
> > > +
> > > + appendPQExpBuffer(str,
> > > + "SELECT lsn FROM
> > pg_catalog.pg_create_logical_replication_slot(%s, 'pgoutput', false, false,
> > false)",
> >
> > This is passing twophase=false, but the patch does not mention prepared
> > transactions. Is the intent to not support workloads containing prepared
> > transactions? If so, the documentation should say that, and the tool likely
> > should warn on startup if max_prepared_transactions != 0.
>
> IIUC, We decided because it is a default behavior of logical replication. See
> [1].
> +1 for improving a documentation, but not sure it is helpful for adding
> output.
> I want to know opinions from others.
>
>
> Documentation says
>
> When two-phase commit is enabled, prepared transactions are sent to the
> subscriber at the time of PREPARE TRANSACTION, and are processed as two-phase
> transactions on the subscriber too. Otherwise, prepared transactions are sent
> to
> the subscriber only when committed, and are then processed immediately by the
> subscriber.
>
> Hence, the replication should be working for prepared transactions even if it
> created the slot with twophase = false. IIRC the user won't be able to change
> it
> later. As Amit said in a previous email, once the command
> ALTER SUBSCRIPTION ... SET (two_phase = on) is supported, users can change it
> after running pg_createsubscriber. The other option is to add a command-line
> option to enable or disable it.
>
Yeah, it is a good idea to add a new option for two_phase but that
should be done in the next version. For now, I suggest updating the
docs and probably raising a warning (if max_prepared_transactions !=
0) as suggested by Noah. This WARNING is useful because one could
expect that setting max_prepared_transactions != 0 means apply will
happen at prepare time after the subscriber is created by this tool.
The WARNING will be useful even if we support two_phase option as the
user may have set the non-zero value of max_prepared_transactions but
didn't use two_phase option.
--
With Regards,
Amit Kapila.
