Date: Sat, 22 Jun 2024 23:21:57 -0700
From: Noah Misch <noah@leadboat.com>
To: Peter Eisentraut <peter@eisentraut.org>
Cc: Euler Taveira <euler@eulerto.com>,
	Amit Kapila <amit.kapila16@gmail.com>,
	Shlok Kyal <shlok.kyal.oss@gmail.com>,
	"kuroda.hayato@fujitsu.com" <kuroda.hayato@fujitsu.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>,
	=?iso-8859-1?Q?Fabr=EDzio?= de Royes Mello <fabriziomello@gmail.com>,
	vignesh C <vignesh21@gmail.com>
Subject: Re: speed up a logical replica setup
Message-ID: <20240623062157.97.nmisch@google.com>
References: <e8564096-3c4f-4990-ac7d-ba089739b878@app.fastmail.com>
 <CANhcyEW6-dH28gLbFc5XpDTJ6JPizU+t5g-aKUWJBf5W_Zriqw@mail.gmail.com>
 <2f0b73f3-e7c0-4e95-9712-1993442a6b64@app.fastmail.com>
 <CAA4eK1JtnzqYNQ_S+mj_XsPfgk7szxegKiVQq4-8fY1wUcUnpg@mail.gmail.com>
 <34637e7f-0330-420d-8f45-1d022962d2fe@app.fastmail.com>
 <7a970912-0b77-4942-84f7-2c9ca0bc05a5@eisentraut.org>
 <d546c4bb-92d1-4e2d-898f-48234b12ed25@app.fastmail.com>
 <CANhcyEWdTTO71v+GFYUTsOOUvHus1qccg88bRmsU9KykJqjSrQ@mail.gmail.com>
 <44de4e04-db22-40ed-8ba9-46387dc8bf83@app.fastmail.com>
 <3fa9ef0f-b277-4c13-850a-8ccc04de1406@eisentraut.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3fa9ef0f-b277-4c13-850a-8ccc04de1406@eisentraut.org>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: <https://www.postgresql.org/message-id/20240623062157.97.nmisch%40google.com>
Precedence: bulk

On Mon, Mar 25, 2024 at 12:55:39PM +0100, Peter Eisentraut wrote:
> I have committed your version v33.

> commit d44032d

> --- /dev/null
> +++ b/src/bin/pg_basebackup/pg_createsubscriber.c

> +static char *
> +concat_conninfo_dbname(const char *conninfo, const char *dbname)
> +{
> +	PQExpBuffer buf = createPQExpBuffer();
> +	char	   *ret;
> +
> +	Assert(conninfo != NULL);
> +
> +	appendPQExpBufferStr(buf, conninfo);
> +	appendPQExpBuffer(buf, " dbname=%s", dbname);

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.

> +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.

> +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@enterprisedb.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?