Обсуждение: Deduplicate logicalrep_read_tuple()
Hi, logicalrep_read_tuple() duplicates code for LOGICALREP_COLUMN_TEXT and LOGICALREP_COLUMN_BINARY introduced by commit 9de77b5. While it doesn't hurt anyone, deduplication makes code a bit leaner by 57 bytes [1]. I've attached a patch for $SUBJECT. Thoughts? [1] size ./src/backend/replication/logical/proto.o PATCHED: text data bss dec hex filename 15558 0 0 15558 3cc6 ./src/backend/replication/logical/proto.o HEAD: text data bss dec hex filename 15615 0 0 15615 3cff ./src/backend/replication/logical/proto.o -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Вложения
On Wed, Jan 18, 2023 at 6:26 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > Hi, > > logicalrep_read_tuple() duplicates code for LOGICALREP_COLUMN_TEXT and > LOGICALREP_COLUMN_BINARY introduced by commit 9de77b5. While it > doesn't hurt anyone, deduplication makes code a bit leaner by 57 bytes > [1]. I've attached a patch for $SUBJECT. > > Thoughts? > The code looks the same but there is a subtle comment difference where previously only LOGICALREP_COLUMN_BINARY case said: /* not strictly necessary but per StringInfo practice */ So if you de-duplicate the code then should that comment be modified to say /* not strictly necessary for LOGICALREP_COLUMN_BINARY but per StringInfo practice */ ------ Kind Regards, Peter Smith. Fujitsu Australia
On Thu, Jan 19, 2023 at 8:36 AM Peter Smith <smithpb2250@gmail.com> wrote: > > On Wed, Jan 18, 2023 at 6:26 PM Bharath Rupireddy > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > Hi, > > > > logicalrep_read_tuple() duplicates code for LOGICALREP_COLUMN_TEXT and > > LOGICALREP_COLUMN_BINARY introduced by commit 9de77b5. While it > > doesn't hurt anyone, deduplication makes code a bit leaner by 57 bytes > > [1]. I've attached a patch for $SUBJECT. > > > > Thoughts? > > > > The code looks the same but there is a subtle comment difference where > previously only LOGICALREP_COLUMN_BINARY case said: > /* not strictly necessary but per StringInfo practice */ > > So if you de-duplicate the code then should that comment be modified to say > /* not strictly necessary for LOGICALREP_COLUMN_BINARY but per > StringInfo practice */ Thanks. Done so in the attached v2. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Вложения
On Fri, Mar 3, 2023 at 4:13 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Thu, Jan 19, 2023 at 8:36 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > > On Wed, Jan 18, 2023 at 6:26 PM Bharath Rupireddy > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > > > Hi, > > > > > > logicalrep_read_tuple() duplicates code for LOGICALREP_COLUMN_TEXT and > > > LOGICALREP_COLUMN_BINARY introduced by commit 9de77b5. While it > > > doesn't hurt anyone, deduplication makes code a bit leaner by 57 bytes > > > [1]. I've attached a patch for $SUBJECT. > > > > > > Thoughts? > > > > > > > The code looks the same but there is a subtle comment difference where > > previously only LOGICALREP_COLUMN_BINARY case said: > > /* not strictly necessary but per StringInfo practice */ > > > > So if you de-duplicate the code then should that comment be modified to say > > /* not strictly necessary for LOGICALREP_COLUMN_BINARY but per > > StringInfo practice */ > > Thanks. Done so in the attached v2. > LGTM. Unless Peter or someone has any comments on this, I'll push this early next week. -- With Regards, Amit Kapila.
On Fri, Mar 3, 2023 at 10:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Fri, Mar 3, 2023 at 4:13 PM Bharath Rupireddy > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > On Thu, Jan 19, 2023 at 8:36 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > > > > On Wed, Jan 18, 2023 at 6:26 PM Bharath Rupireddy > > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > > > > > Hi, > > > > > > > > logicalrep_read_tuple() duplicates code for LOGICALREP_COLUMN_TEXT and > > > > LOGICALREP_COLUMN_BINARY introduced by commit 9de77b5. While it > > > > doesn't hurt anyone, deduplication makes code a bit leaner by 57 bytes > > > > [1]. I've attached a patch for $SUBJECT. > > > > > > > > Thoughts? > > > > > > > > > > The code looks the same but there is a subtle comment difference where > > > previously only LOGICALREP_COLUMN_BINARY case said: > > > /* not strictly necessary but per StringInfo practice */ > > > > > > So if you de-duplicate the code then should that comment be modified to say > > > /* not strictly necessary for LOGICALREP_COLUMN_BINARY but per > > > StringInfo practice */ > > > > Thanks. Done so in the attached v2. > > > > LGTM. Unless Peter or someone has any comments on this, I'll push this > early next week. > No more comments. Patch v2 LGTM. ------ Kind Regards, Peter Smith. Fujitsu Australia
On Sun, Mar 5, 2023 at 1:22 PM Peter Smith <smithpb2250@gmail.com> wrote: > > On Fri, Mar 3, 2023 at 10:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > > > > Thanks. Done so in the attached v2. > > > > > > > LGTM. Unless Peter or someone has any comments on this, I'll push this > > early next week. > > > > No more comments. Patch v2 LGTM. > Pushed. -- With Regards, Amit Kapila.