Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1pKSkU-00054K-RQ for pgsql-hackers@arkaria.postgresql.org; Tue, 24 Jan 2023 23:30:43 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1pKSkT-0007LG-Bl for pgsql-hackers@arkaria.postgresql.org; Tue, 24 Jan 2023 23:30:41 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1pKSkS-0007L7-W7 for pgsql-hackers@lists.postgresql.org; Tue, 24 Jan 2023 23:30:41 +0000 Received: from mail-lf1-x133.google.com ([2a00:1450:4864:20::133]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1pKSkQ-0003xL-Ef for pgsql-hackers@lists.postgresql.org; Tue, 24 Jan 2023 23:30:40 +0000 Received: by mail-lf1-x133.google.com with SMTP id g13so26074114lfv.7 for ; Tue, 24 Jan 2023 15:30:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=3S8TVBf0Fr4WV3pVnt4E/E0UYX1uIV9hu+zripBRpCQ=; b=KjKviWjObw0VTaPAnLB0oS601c80xGYjTIHN1TYmcLRvW4KFEH34nU6Dfg52Nm75tq ocyQeuvThRO2Z5IhY/uAjpFUOpEJQeJZf/J4MCx47Fxjtv8Z2VFXKFjzJ5eFrA0AtqDP /5WUiYoEhbJs96o+5C8qp/lXaeJ8VvSDkWhGXD3fd0sGFxjF0XeZrlZyc3t05j2AiPwn Uu/XAsQ9kVp3yH89ldAnmwEZqPCv20IRI+YXpFHVNAz88PJAMf4cUKKTR8gYmFuUMdZG D2jvzom9TpNBAYR6rm4tCISSjP9S4Tgo6OvFn8N8AjHBKNpQzQuKoK0u08EpV4qrUeA5 7BmQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=3S8TVBf0Fr4WV3pVnt4E/E0UYX1uIV9hu+zripBRpCQ=; b=NTZxh4h0fgqTd7ZyUn8QBCkAQMG9e6AUbwVgeTc7adNRaS7NmC/qdn22PkB0JfSBJ+ BWUo2n73vxRFDkirBTfuxyoVO8VKpOYGB8VqVv59AsQ+1UejSpp9o20muUSRpihDWbZb lwjv1vRI1/Vle+9Q3GbU9AKJt3oNOvHQI1wl59tp/CdbSeleIdo3qTM7Z6IbZecx9Ue3 gwn/BAZ2mrlroYJEtFWIh7gM8FKwJpn2Ru+H3A6tMvv6fpyr5Jt3gyPM5C1QgDZ+91ra uyVwl1q4yYhhB9QPeGn8K7n4p5iBw/LOzLPKAorXeQMSVOaT2JWjCyv5SLbBqbqp+cPI zAlA== X-Gm-Message-State: AFqh2krqKb4Y541i5eoSW5/7zhyIfmMpG4dnOKwdPnppLppx3Y2BFzOl xhCDo8t21GVsO+B3bMe+c705IQUfVvb8rHvSQPE= X-Google-Smtp-Source: AMrXdXs2K41dLw0euSpA4yFkbGG1YLWDGe22YSL2qgejurj16R4rmbpBj9iNLOaIGlt2YVUnAmbmTn4qpncuf9dxelc= X-Received: by 2002:a05:6512:2345:b0:4cc:93e6:5ebb with SMTP id p5-20020a056512234500b004cc93e65ebbmr1649828lfu.585.1674603036816; Tue, 24 Jan 2023 15:30:36 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Peter Smith Date: Wed, 25 Jan 2023 10:30:19 +1100 Message-ID: Subject: Re: Perform streaming logical transactions by background workers and parallel apply To: "houzj.fnst@fujitsu.com" Cc: Amit Kapila , shveta malik , "wangw.fnst@fujitsu.com" , "shiy.fnst@fujitsu.com" , PostgreSQL Hackers , Dilip Kumar , Masahiko Sawada Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk Here are my review comments for patch v87-0002. =3D=3D=3D=3D=3D=3D doc/src/sgml/config.sgml 1. - Allows streaming or serializing changes immediately in logical decoding. The allowed values of logical_replication_mode = are - buffered and immediate. When= set - to immediate, stream each change if + buffered and immediate. The default + is buffered. + I didn't think it was necessary to say =E2=80=9Cof logical_replication_mode= =E2=80=9D. IMO that much is already obvious because this is the first sentence of the description for logical_replication_mode. (see also review comment #4) ~~~ 2. + + On the publisher side, it allows streaming or serializing changes + immediately in logical decoding. When set to + immediate, stream each change if streaming option (see optional parameters set b= y CREATE SUBSCRIPTION) is enabled, otherwise, serialize each change. When set to - buffered, which is the default, decoding will s= tream - or serialize changes when logical_decoding_work_mem - is reached. + buffered, decoding will stream or serialize cha= nges + when logical_decoding_work_mem is reached. 2a. "it allows" --> "logical_replication_mode allows" 2b. "decoding" --> "the decoding" ~~~ 3. + + On the subscriber side, if streaming option is = set + to parallel, this parameter also allows the lea= der + apply worker to send changes to the shared memory queue or to seri= alize + changes. When set to buffered, the leader send= s + changes to parallel apply workers via shared memory queue. When s= et to + immediate, the leader serializes all changes to + files and notifies the parallel apply workers to read and apply th= em at + the end of the transaction. + "this parameter also allows" --> "logical_replication_mode also allows" ~~~ 4. This parameter is intended to be used to test logical decoding and replication of large transactions for which otherwise we need to generate the changes till logical_decoding_work_mem - is reached. + is reached. Moreover, this can also be used to test the transmissi= on of + changes between the leader and parallel apply workers. "Moreover, this can also" --> "It can also" I am wondering would this sentence be better put at the top of the GUC description. So then the first paragraph becomes like this: SUGGESTION (I've also added another sentence "The effect of...") The allowed values are buffered and immediate. The default is buffered. This parameter is intended to be used to test logical decoding and replication of large transactions for which otherwise we need to generate the changes till logical_decoding_work_mem is reached. It can also be used to test the transmission of changes between the leader and parallel apply workers. The effect of logical_replication_mode is different for the publisher and subscriber: On the publisher side... On the subscriber side... =3D=3D=3D=3D=3D=3D .../replication/logical/applyparallelworker.c 5. + /* + * In immeidate mode, directly return false so that we can switch to + * PARTIAL_SERIALIZE mode and serialize remaining changes to files. + */ + if (logical_replication_mode =3D=3D LOGICAL_REP_MODE_IMMEDIATE) + return false; Typo "immediate" Also, I felt "directly" is not needed. "return false" and "directly return false" is the same. SUGGESTION Using =E2=80=98immediate=E2=80=99 mode returns false to cause a switch to PARTIAL_SERIALIZE mode so that the remaining changes will be serialized. =3D=3D=3D=3D=3D=3D src/backend/utils/misc/guc_tables.c 6. { {"logical_replication_mode", PGC_USERSET, DEVELOPER_OPTIONS, - gettext_noop("Allows streaming or serializing each change in logical decoding."), - NULL, + gettext_noop("Controls the behavior of logical replication publisher and subscriber"), + gettext_noop("If set to immediate, on the publisher side, it " + "allows streaming or serializing each change in " + "logical decoding. On the subscriber side, in " + "parallel streaming mode, it allows the leader apply " + "worker to serialize changes to files and notifies " + "the parallel apply workers to read and apply them at " + "the end of the transaction."), GUC_NOT_IN_SAMPLE }, 6a. short description User PoV behaviour should be the same. Instead, maybe say "controls the internal behavior" or something like that? ~ 6b. long description IMO the long description shouldn=E2=80=99t mention =E2=80=98immediate=E2=80= =99 mode first as it does. BEFORE If set to immediate, on the publisher side, ... AFTER On the publisher side, ... ------ Kind Regards, Peter Smith. Fujitsu Australia