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]: 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
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
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
Attachments:
v1-0001-Deduplicate-logicalrep_read_tuple.patchapplication/x-patch; name=v1-0001-Deduplicate-logicalrep_read_tuple.patchDownload
From 9c645a11a56b3729b4f9184e6bfb7a6fcf834692 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Wed, 18 Jan 2023 05:14:47 +0000
Subject: [PATCH v1] Deduplicate logicalrep_read_tuple()
---
src/backend/replication/logical/proto.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/src/backend/replication/logical/proto.c b/src/backend/replication/logical/proto.c
index 3a9d53a61e..86e0750fde 100644
--- a/src/backend/replication/logical/proto.c
+++ b/src/backend/replication/logical/proto.c
@@ -895,17 +895,6 @@ logicalrep_read_tuple(StringInfo in, LogicalRepTupleData *tuple)
/* we don't receive the value of an unchanged column */
break;
case LOGICALREP_COLUMN_TEXT:
- len = pq_getmsgint(in, 4); /* read length */
-
- /* and data */
- value->data = palloc(len + 1);
- pq_copymsgbytes(in, value->data, len);
- value->data[len] = '\0';
- /* make StringInfo fully valid */
- value->len = len;
- value->cursor = 0;
- value->maxlen = len;
- break;
case LOGICALREP_COLUMN_BINARY:
len = pq_getmsgint(in, 4); /* read length */
--
2.34.1
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
Attachments:
v2-0001-Deduplicate-logicalrep_read_tuple.patchapplication/octet-stream; name=v2-0001-Deduplicate-logicalrep_read_tuple.patchDownload
From 49f64ce6b476ae9f5506f44d4f1bc53174670ba1 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Fri, 3 Mar 2023 10:42:07 +0000
Subject: [PATCH v2] Deduplicate logicalrep_read_tuple()
---
src/backend/replication/logical/proto.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/src/backend/replication/logical/proto.c b/src/backend/replication/logical/proto.c
index 3a9d53a61e..7cfc4e847a 100644
--- a/src/backend/replication/logical/proto.c
+++ b/src/backend/replication/logical/proto.c
@@ -895,25 +895,19 @@ logicalrep_read_tuple(StringInfo in, LogicalRepTupleData *tuple)
/* we don't receive the value of an unchanged column */
break;
case LOGICALREP_COLUMN_TEXT:
- len = pq_getmsgint(in, 4); /* read length */
-
- /* and data */
- value->data = palloc(len + 1);
- pq_copymsgbytes(in, value->data, len);
- value->data[len] = '\0';
- /* make StringInfo fully valid */
- value->len = len;
- value->cursor = 0;
- value->maxlen = len;
- break;
case LOGICALREP_COLUMN_BINARY:
len = pq_getmsgint(in, 4); /* read length */
/* and data */
value->data = palloc(len + 1);
pq_copymsgbytes(in, value->data, len);
- /* not strictly necessary but per StringInfo practice */
+
+ /*
+ * Not strictly necessary for LOGICALREP_COLUMN_BINARY, but per
+ * StringInfo practice.
+ */
value->data[len] = '\0';
+
/* make StringInfo fully valid */
value->len = len;
value->cursor = 0;
--
2.34.1
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.