Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1sMHpI-001Vey-HA for pgsql-hackers@arkaria.postgresql.org; Wed, 26 Jun 2024 01:52:01 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1sMHpG-001Vvu-Ov for pgsql-hackers@arkaria.postgresql.org; Wed, 26 Jun 2024 01:51:59 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1sMHpF-001Vvm-Bb for pgsql-hackers@lists.postgresql.org; Wed, 26 Jun 2024 01:51:58 +0000 Received: from fhigh6-smtp.messagingengine.com ([103.168.172.157]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1sMHpC-0039ED-CB for pgsql-hackers@lists.postgresql.org; Wed, 26 Jun 2024 01:51:56 +0000 Received: from compute2.internal (compute2.nyi.internal [10.202.2.46]) by mailfhigh.nyi.internal (Postfix) with ESMTP id AC13211401D0; Tue, 25 Jun 2024 21:51:52 -0400 (EDT) Received: from imap49 ([10.202.2.99]) by compute2.internal (MEProxy); Tue, 25 Jun 2024 21:51:52 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=eulerto.com; h= cc:cc:content-type:content-type:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:subject :subject:to:to; s=fm3; t=1719366712; x=1719453112; bh=aNA3cJ2zZd C+O/eXEAN9RVsDoaU2uaRKSupKRnnYgC4=; b=R61D6+4jyeHrL372wnkAuwIyIc d+8IJ55z+PX8NpnrrmDmu5q0QOt0HnLoV1PVDJRrxJrmkoeGOfEVTgQt5r7IKW/L 8S3DAklYHxD0PZM/JEDaKjxv223oQwVCry13MCme6rNl6PF4aJa/uKpa2uBI2V9h RdSemsfv1Ynni83HEi6WS6xJlEEfx46No8VuEo8eAlveq8gec86gPIlfZa44Ihth 60zhQvzcqqMtApnCz56WHALT4AeRH/ydXJLW+KZEA9BCxPrGRuEZ0bWgKJoHLL1P O4zBZy2TKi2Y/ym2OjsmR9bWUUr4SzG1juG9m6Y/itzdLj6R8RucHZoSkowQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:subject:subject:to :to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm2; t=1719366712; x=1719453112; bh=aNA3cJ2zZdC+O/eXEAN9RVsDoaU2 uaRKSupKRnnYgC4=; b=PB71CeORtmQyEZzRXVECeOUHW/thQqMT+r3CIw1Jui4/ aqwm8PcXyu8leEPH3rVFQMqyDj8GGJ1k80wVM9wlOFFp02kIVqoH1Rm351pqbbKL VZplJvovexDXKU/MPiJifwt+a0C/pJFz/0siYXR5RzC6+Oe6ib72LtEP4yMjk6bs d2/xFYg3wlmWfRne+VI295bN6ddmHVwbcDHcXxXLRBW81UkQaBDV57ml+79SuRTo AE+XCATEE9D2oXRa3ZbOsamsDP6/Tf//p++iu1MDrvHmxim/L5p59V4OQu9TVevw PyPDFnj0W942xwFg7FZqTuzEtNZC9ZOvfjvoSneRxw== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeeftddrtddugdehudcutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefofgggkfgjfhffhffvvefutgesrgdtreerreertdenucfhrhhomhepfdfguhhl vghrucfvrghvvghirhgrfdcuoegvuhhlvghrsegvuhhlvghrthhordgtohhmqeenucggtf frrghtthgvrhhnpefgffejvdeivdekvdeivdetudeikeduieevtdfhhfduvdetgeelteei ffekgefhvdenucffohhmrghinhepvghnthgvrhhprhhishgvuggsrdgtohhmnecuvehluh hsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepvghulhgvrhesvghu lhgvrhhtohdrtghomh X-ME-Proxy: Feedback-ID: i0c21471d:Fastmail Received: by mailuser.nyi.internal (Postfix, from userid 501) id 4E28715A0092; Tue, 25 Jun 2024 21:51:51 -0400 (EDT) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.11.0-alpha0-538-g1508afaa2-fm-20240616.001-g1508afaa MIME-Version: 1.0 Message-Id: <34a8a515-a2b4-49e1-839c-4ed4e07a77b8@app.fastmail.com> In-Reply-To: References: <2f0b73f3-e7c0-4e95-9712-1993442a6b64@app.fastmail.com> <34637e7f-0330-420d-8f45-1d022962d2fe@app.fastmail.com> <7a970912-0b77-4942-84f7-2c9ca0bc05a5@eisentraut.org> <44de4e04-db22-40ed-8ba9-46387dc8bf83@app.fastmail.com> <3fa9ef0f-b277-4c13-850a-8ccc04de1406@eisentraut.org> <20240623062157.97.nmisch@google.com> Date: Tue, 25 Jun 2024 22:51:31 -0300 From: "Euler Taveira" To: "kuroda.hayato@fujitsu.com" , "Noah Misch" , "Peter Eisentraut" Cc: "Amit Kapila" , "Shlok Kyal" , "Tomas Vondra" , "pgsql-hackers@lists.postgresql.org" , "Michael Paquier" , "Andres Freund" , "Ashutosh Bapat" , =?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= , "vignesh C" Subject: Re: speed up a logical replica setup Content-Type: multipart/alternative; boundary=a0bfc7e005af44ccad7bf7bb48608c26 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --a0bfc7e005af44ccad7bf7bb48608c26 Content-Type: text/plain 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. -- Euler Taveira EDB https://www.enterprisedb.com/ --a0bfc7e005af44ccad7bf7bb48608c26 Content-Type: text/html Content-Transfer-Encoding: quoted-printable
On Mon, Jun 24,= 2024, at 3:47 AM, Hayato Kuroda (Fujitsu) wrote:
> pg_createsubscriber fails on= a dbname containing a space.  Use
> appendConnStr= Val() 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 ge= nerate_db() in the pg_upgrade test suite.

T= hanks for pointing out. I made a fix patch. Test code was also modified = accordingly.

Patch looks good = to me. I have one suggestion

-# Mostly Port= ed from 002_pg_upgrade.pl, but this returns a generated dbname.
+# Extracted from 002_pg_upgrade.pl.

= and 2 small fixes:

-   'logical r= eplication works on database $db1');
+   "logica= l replication works on database $db1");

-is= ($result, qq(row 1), 'logical replication works on database $db2');
<= /div>
+is($result, qq(row 1), "logical replication works on database= $db2");

> > +static char *
> > +create_lo= gical_replication_slot(PGconn *conn, struct LogicalRepInfo *dbinfo)
<= /div>
> > +{
> > + PQExpBuffer str =3D cre= atePQExpBuffer();
> > + PGresult   *res =3D= NULL;
> > + const char *slot_name =3D dbinfo->re= plslotname;
> > + char    *slot_name_esc;<= br>
> > + char    *lsn =3D NULL;
> > +
> > + Assert(conn !=3D NULL);
=
> > +
> > + pg_log_info("creating the rep= lication slot \"%s\" on database \"%s\"",
> > + s= lot_name, dbinfo->dbname);
> > +
&g= t; > + slot_name_esc =3D PQescapeLiteral(conn, slot_name, strlen(slot= _name));
> > +
> > + appendPQExp= Buffer(str,
> > +   "SELECT lsn FROM
> pg_catalog.pg_create_logical_replication_slot(%s, 'pgoutput= ', false, false, false)",
> 
> Th= is is passing twophase=3Dfalse, but the patch does not mention prepared<= br>
> transactions.  Is the intent to not support work= loads containing prepared
> transactions?  If so, = the documentation should say that, and the tool likely
>= ; should warn on startup if max_prepared_transactions !=3D 0.
<= div>
IIUC, We decided because it is a default behavior of = logical replication. See [1].
+1 for improving a documenta= tion, but not sure it is helpful for adding output.
I want= to know opinions from others.

Documentation says

When two-phase commit i= s enabled, prepared transactions are sent to the
subscribe= r at the time of PREPARE TRANSACTION, and are processed as two-phase
=
transactions on the subscriber too. Otherwise, prepared trans= actions are sent to
the subscriber only when committed, an= d are then processed immediately by the
subscriber.

Hence, the replication should be working for prep= ared transactions even if it
created the slot with twophas= e =3D false. IIRC the user won't be able to change it
late= r. As Amit said in a previous email, once the command
ALTE= R SUBSCRIPTION ... SET (two_phase =3D 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.
=


--
Euler Taveira
=
EDB   https://www.enterprisedb.com/

--a0bfc7e005af44ccad7bf7bb48608c26--