Narrow the scope of the variable outputstr in logicalrep_write_tuple

Started by japinalmost 5 years ago5 messages

japinli@hotmail.com

almost 5 years ago

1 attachment(s)

Hi,

I find that the outputstr variable in logicalrep_write_tuple() only use in
`else` branch, I think we can narrow the scope, just like variable outputbytes
in `if` branch (for more readable).

/*
* Send in binary if requested and type has suitable send function.
*/
if (binary && OidIsValid(typclass->typsend))
{
bytea *outputbytes;
int len;

pq_sendbyte(out, LOGICALREP_COLUMN_BINARY);
outputbytes = OidSendFunctionCall(typclass->typsend, values[i]);
len = VARSIZE(outputbytes) - VARHDRSZ;
pq_sendint(out, len, 4); /* length */
pq_sendbytes(out, VARDATA(outputbytes), len); /* data */
pfree(outputbytes);
}
else
{
pq_sendbyte(out, LOGICALREP_COLUMN_TEXT);
outputstr = OidOutputFunctionCall(typclass->typoutput, values[i]);
pq_sendcountedtext(out, outputstr, strlen(outputstr), false);
pfree(outputstr);
}

Attached is a samll patch to fix it.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Attachments:

narrow-the-scope-of-the-variable-in-logicalrep_write_tuple.patchtext/x-patchDownload

diff --git a/src/backend/replication/logical/proto.c b/src/backend/replication/logical/proto.c
index 62275ebabe..f2c85cabb5 100644
--- a/src/backend/replication/logical/proto.c
+++ b/src/backend/replication/logical/proto.c
@@ -493,7 +493,6 @@ logicalrep_write_tuple(StringInfo out, Relation rel, HeapTuple tuple, bool binar
                HeapTuple       typtup;
                Form_pg_type typclass;
                Form_pg_attribute att = TupleDescAttr(desc, i);
-               char       *outputstr;

                if (att->attisdropped || att->attgenerated)
                        continue;
@@ -537,6 +536,8 @@ logicalrep_write_tuple(StringInfo out, Relation rel, HeapTuple tuple, bool binar
                }
                else
                {
+                       char       *outputstr;
+
                        pq_sendbyte(out, LOGICALREP_COLUMN_TEXT);
                        outputstr = OidOutputFunctionCall(typclass->typoutput, values[i]);
                        pq_sendcountedtext(out, outputstr, strlen(outputstr), false);

Bharath Rupireddy

bharath.rupireddyforpostgres@gmail.com

almost 5 years ago

In reply to: japin (#1)

Re: Narrow the scope of the variable outputstr in logicalrep_write_tuple

On Mon, Jan 18, 2021 at 1:16 PM japin <japinli@hotmail.com> wrote:

Hi,

I find that the outputstr variable in logicalrep_write_tuple() only use in
`else` branch, I think we can narrow the scope, just like variable outputbytes
in `if` branch (for more readable).

/*
* Send in binary if requested and type has suitable send function.
*/
if (binary && OidIsValid(typclass->typsend))
{
bytea *outputbytes;
int len;

pq_sendbyte(out, LOGICALREP_COLUMN_BINARY);
outputbytes = OidSendFunctionCall(typclass->typsend, values[i]);
len = VARSIZE(outputbytes) - VARHDRSZ;
pq_sendint(out, len, 4); /* length */
pq_sendbytes(out, VARDATA(outputbytes), len); /* data */
pfree(outputbytes);
}
else
{
pq_sendbyte(out, LOGICALREP_COLUMN_TEXT);
outputstr = OidOutputFunctionCall(typclass->typoutput, values[i]);
pq_sendcountedtext(out, outputstr, strlen(outputstr), false);
pfree(outputstr);
}

Attached is a samll patch to fix it.

+1. Binary mode uses block level variable outputbytes, so making
outputstr block level is fine IMO.

Patch basically looks good to me, but it doesn't apply on my system.
Looks like it's not created with git commit. Please create the patch
with git commit command.

git apply /mnt/hgfs/Shared/narrow-the-scope-of-the-variable-in-logicalrep_write_tuple.patch
error: corrupt patch at line 10

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

japin

japinli@hotmail.com

almost 5 years ago

In reply to: Bharath Rupireddy (#2)

1 attachment(s)

Re: Narrow the scope of the variable outputstr in logicalrep_write_tuple

On Mon, 18 Jan 2021 at 15:59, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

On Mon, Jan 18, 2021 at 1:16 PM japin <japinli@hotmail.com> wrote:

Hi,

I find that the outputstr variable in logicalrep_write_tuple() only use in
`else` branch, I think we can narrow the scope, just like variable outputbytes
in `if` branch (for more readable).

/*
* Send in binary if requested and type has suitable send function.
*/
if (binary && OidIsValid(typclass->typsend))
{
bytea *outputbytes;
int len;

pq_sendbyte(out, LOGICALREP_COLUMN_BINARY);
outputbytes = OidSendFunctionCall(typclass->typsend, values[i]);
len = VARSIZE(outputbytes) - VARHDRSZ;
pq_sendint(out, len, 4); /* length */
pq_sendbytes(out, VARDATA(outputbytes), len); /* data */
pfree(outputbytes);
}
else
{
pq_sendbyte(out, LOGICALREP_COLUMN_TEXT);
outputstr = OidOutputFunctionCall(typclass->typoutput, values[i]);
pq_sendcountedtext(out, outputstr, strlen(outputstr), false);
pfree(outputstr);
}

Attached is a samll patch to fix it.

+1. Binary mode uses block level variable outputbytes, so making
outputstr block level is fine IMO.

Patch basically looks good to me, but it doesn't apply on my system.
Looks like it's not created with git commit. Please create the patch
with git commit command.

git apply /mnt/hgfs/Shared/narrow-the-scope-of-the-variable-in-logicalrep_write_tuple.patch
error: corrupt patch at line 10

Thanks for reviewing! Attached v2 as you suggested.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Attachments:

v2-0001-Narrow-the-scope-of-the-variable-outputstr-in-log.patchtext/x-patchDownload

From 89fb10efc9eccf11f531ab8675376b57a12a6cb1 Mon Sep 17 00:00:00 2001
From: Japin Li <japinli@hotmail.com>
Date: Mon, 18 Jan 2021 16:04:54 +0800
Subject: [PATCH v2] Narrow the scope of the variable outputstr in
 logicalrep_write_tuple

---
 src/backend/replication/logical/proto.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/backend/replication/logical/proto.c b/src/backend/replication/logical/proto.c
index 62275ebabe..f2c85cabb5 100644
--- a/src/backend/replication/logical/proto.c
+++ b/src/backend/replication/logical/proto.c
@@ -493,7 +493,6 @@ logicalrep_write_tuple(StringInfo out, Relation rel, HeapTuple tuple, bool binar
 		HeapTuple	typtup;
 		Form_pg_type typclass;
 		Form_pg_attribute att = TupleDescAttr(desc, i);
-		char	   *outputstr;
 
 		if (att->attisdropped || att->attgenerated)
 			continue;
@@ -537,6 +536,8 @@ logicalrep_write_tuple(StringInfo out, Relation rel, HeapTuple tuple, bool binar
 		}
 		else
 		{
+			char	   *outputstr;
+
 			pq_sendbyte(out, LOGICALREP_COLUMN_TEXT);
 			outputstr = OidOutputFunctionCall(typclass->typoutput, values[i]);
 			pq_sendcountedtext(out, outputstr, strlen(outputstr), false);
-- 
2.30.0

Bharath Rupireddy

bharath.rupireddyforpostgres@gmail.com

almost 5 years ago

In reply to: japin (#3)

Re: Narrow the scope of the variable outputstr in logicalrep_write_tuple

On Mon, Jan 18, 2021 at 1:39 PM japin <japinli@hotmail.com> wrote:

On Mon, 18 Jan 2021 at 15:59, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

On Mon, Jan 18, 2021 at 1:16 PM japin <japinli@hotmail.com> wrote:

Hi,

I find that the outputstr variable in logicalrep_write_tuple() only use in
`else` branch, I think we can narrow the scope, just like variable outputbytes
in `if` branch (for more readable).

/*
* Send in binary if requested and type has suitable send function.
*/
if (binary && OidIsValid(typclass->typsend))
{
bytea *outputbytes;
int len;

pq_sendbyte(out, LOGICALREP_COLUMN_BINARY);
outputbytes = OidSendFunctionCall(typclass->typsend, values[i]);
len = VARSIZE(outputbytes) - VARHDRSZ;
pq_sendint(out, len, 4); /* length */
pq_sendbytes(out, VARDATA(outputbytes), len); /* data */
pfree(outputbytes);
}
else
{
pq_sendbyte(out, LOGICALREP_COLUMN_TEXT);
outputstr = OidOutputFunctionCall(typclass->typoutput, values[i]);
pq_sendcountedtext(out, outputstr, strlen(outputstr), false);
pfree(outputstr);
}

Attached is a samll patch to fix it.

+1. Binary mode uses block level variable outputbytes, so making
outputstr block level is fine IMO.

Patch basically looks good to me, but it doesn't apply on my system.
Looks like it's not created with git commit. Please create the patch
with git commit command.

git apply /mnt/hgfs/Shared/narrow-the-scope-of-the-variable-in-logicalrep_write_tuple.patch
error: corrupt patch at line 10

Thanks for reviewing! Attached v2 as you suggested.

Thanks. v2 patch LGTM.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

Tom Lane

tgl@sss.pgh.pa.us

almost 5 years ago

In reply to: japin (#1)

Re: Narrow the scope of the variable outputstr in logicalrep_write_tuple

japin <japinli@hotmail.com> writes:

I find that the outputstr variable in logicalrep_write_tuple() only use in
`else` branch, I think we can narrow the scope, just like variable outputbytes
in `if` branch (for more readable).

Agreed, done.

For context, I'm not usually in favor of making one-off stylistic
improvements: the benefit seldom outweighs the risk of creating
merge hazards for future back-patching. But in this case, the
code involved is mostly new in v14, so improving it now doesn't
cost anything in that way.

regards, tom lane