| Lists: | pgsql-hackers |
|---|
| From: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
|---|---|
| To: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Deduplicate logicalrep_read_tuple() |
| Date: | 2023-01-18 07:26:06 |
| Message-ID: | CALj2ACXdbq7kW_+bRrSGMsR6nefCvwbHBJ5J51mr3gFf7QysTA@mail.gmail.com |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers |
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
| Attachment | Content-Type | Size |
|---|---|---|
| v1-0001-Deduplicate-logicalrep_read_tuple.patch | application/x-patch | 1.1 KB |
| From: | Peter Smith <smithpb2250(at)gmail(dot)com> |
|---|---|
| To: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
| Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Deduplicate logicalrep_read_tuple() |
| Date: | 2023-01-19 03:06:26 |
| Message-ID: | CAHut+PtbhRdDpL9twoCzcys8TS2R7b3hzTQBB_cU=A62UWvc8w@mail.gmail.com |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers |
On Wed, Jan 18, 2023 at 6:26 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)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
| From: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
|---|---|
| To: | Peter Smith <smithpb2250(at)gmail(dot)com> |
| Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Deduplicate logicalrep_read_tuple() |
| Date: | 2023-03-03 10:43:23 |
| Message-ID: | CALj2ACV1N+_HvTVxWmCe5qBd3mUR0rWr-Q_7mBLipdz62O19NQ@mail.gmail.com |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers |
On Thu, Jan 19, 2023 at 8:36 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Wed, Jan 18, 2023 at 6:26 PM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)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
| Attachment | Content-Type | Size |
|---|---|---|
| v2-0001-Deduplicate-logicalrep_read_tuple.patch | application/octet-stream | 1.5 KB |
| From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
|---|---|
| To: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
| Cc: | Peter Smith <smithpb2250(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Deduplicate logicalrep_read_tuple() |
| Date: | 2023-03-03 11:04:30 |
| Message-ID: | CAA4eK1KTPUw0f=9fzDr31i1qeLm9yTXp0abu5TPbFdjNM-zyyA@mail.gmail.com |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers |
On Fri, Mar 3, 2023 at 4:13 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Thu, Jan 19, 2023 at 8:36 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > On Wed, Jan 18, 2023 at 6:26 PM Bharath Rupireddy
> > <bharath(dot)rupireddyforpostgres(at)gmail(dot)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.
| From: | Peter Smith <smithpb2250(at)gmail(dot)com> |
|---|---|
| To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
| Cc: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Deduplicate logicalrep_read_tuple() |
| Date: | 2023-03-05 07:52:19 |
| Message-ID: | CAHut+Ptdzy4RcfuFhTn5vWMnNUm-0b_rqtaB3CwMeOK8g+3GLg@mail.gmail.com |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers |
On Fri, Mar 3, 2023 at 10:04 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, Mar 3, 2023 at 4:13 PM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> >
> > On Thu, Jan 19, 2023 at 8:36 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > >
> > > On Wed, Jan 18, 2023 at 6:26 PM Bharath Rupireddy
> > > <bharath(dot)rupireddyforpostgres(at)gmail(dot)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
| From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
|---|---|
| To: | Peter Smith <smithpb2250(at)gmail(dot)com> |
| Cc: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Deduplicate logicalrep_read_tuple() |
| Date: | 2023-03-06 10:44:24 |
| Message-ID: | CAA4eK1LmAxThZ-Hk3DtEHdLS1QanBGji_LQhDgHmQBxmHqQv+Q@mail.gmail.com |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers |
On Sun, Mar 5, 2023 at 1:22 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Fri, Mar 3, 2023 at 10:04 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.