Dear Noah, > 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.
> > +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.
> > +static void
> > +create_publication(PGconn *conn, struct LogicalRepInfo *dbinfo)
> > +{
>
> > + appendPQExpBuffer(str, "CREATE PUBLICATION %s FOR ALL TABLES",
> > + ipubname_esc);
>
> This tool's documentation says it "guarantees that no transaction will be
> lost." I tried to determine whether achieving that will require something
> like the fix from
> https://postgr.es/m/flat/de52b282-1166-1180-45a2-8d8917ca74c6@enterprise
> db.com.
> (Not exactly the fix from that thread, since that thread has not discussed the
> FOR ALL TABLES version of its race condition.) I don't know. On the one
> hand, pg_createsubscriber benefits from creating a logical slot after creating
> the publication. That snapbuild.c process will wait for running XIDs. On the
> other hand, an INSERT/UPDATE/DELETE acquires its RowExclusiveLock and
> builds
> its relcache entry before assigning an XID, so perhaps the snapbuild.c process
> isn't enough to prevent that thread's race condition. What do you think?
IIUC, documentation just intended to say that a type of replication will be
switched from stream to logical one, at the certain point. Please give sometime
for analyzing.
[1]:
https://www.postgresql.org/message-id/270ad9b8-9c46-40c3-b6c5-3d25b91d3a7d%40app.fastmail.com
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/
0001-pg_createsubscriber-Fix-cases-which-connection-param.patch
Description: 0001-pg_createsubscriber-Fix-cases-which-connection-param.patch
