Horton

RE: speed up a logical replica setup

Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>

View thread
From: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
To: 'Noah Misch' <noah@leadboat.com>, Peter Eisentraut <peter@eisentraut.org>
Cc: Euler Taveira <euler@eulerto.com>, Amit Kapila <amit.kapila16@gmail.com>, Shlok Kyal <shlok.kyal.oss@gmail.com>, Tomas Vondra <tomas.vondra@enterprisedb.com>, "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>, Michael Paquier <michael@paquier.xyz>, Andres Freund <andres@anarazel.de>, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, Fabrízio de Royes Mello <fabriziomello@gmail.com>, vignesh C <vignesh21@gmail.com>
Date: 2024-06-24T06:47:54Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. pg_createsubscriber: Remove obsolete comment

  2. pg_createsubscriber: Fix an unpredictable recovery wait time.

  3. Fix unstable test in 040_pg_createsubscriber.

  4. Fix the testcase introduced in commit 81d20fbf7a.

  5. Further weaken new pg_createsubscriber test on Windows.

  6. Temporarily(?) weaken new pg_createsubscriber test on Windows.

  7. Make pg_createsubscriber warn if publisher has two-phase commit enabled.

  8. Make pg_createsubscriber more wary about quoting connection parameters.

  9. pg_createsubscriber: Remove failover replication slots on subscriber

  10. pg_createsubscriber: Remove replication slot check on primary

  11. pg_createsubscriber: Only --recovery-timeout controls the end of recovery process

  12. pg_createsubscriber: creates a new logical replica from a standby server

  13. Add some const decorations

  14. Add option force_initdb to PostgreSQL::Test::Cluster:init()

  15. Remove MSVC scripts

Attachments

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/