Typo in xact.c

Started by Japin Liover 3 years ago6 messages
#1Japin Li
japinli@hotmail.com
1 attachment(s)

Hi hacker,

Recently, I find there might be a typo in xact.c comments. The comments
say "PG_PROC", however, it actually means "PGPROC" structure. Since we
have pg_proc catalog, and use PG_PROC to reference the catalog [1]src/include/nodes/primnodes.h, so,
we should use PGPROC to reference the structure. Any thoughts?

[1]: src/include/nodes/primnodes.h

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

Attachments:

fix-a-typo-in-xact.patchtext/x-patchDownload
diff --git a/src/backend/access/transam/README b/src/backend/access/transam/README
index 734c39a4d0..72af656060 100644
--- a/src/backend/access/transam/README
+++ b/src/backend/access/transam/README
@@ -198,7 +198,7 @@ parent.  This maintains the invariant that child transactions have XIDs later
 than their parents, which is assumed in a number of places.
 
 The subsidiary actions of obtaining a lock on the XID and entering it into
-pg_subtrans and PG_PROC are done at the time it is assigned.
+pg_subtrans and PGPROC are done at the time it is assigned.
 
 A transaction that has no XID still needs to be identified for various
 purposes, notably holding locks.  For this purpose we assign a "virtual
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 50f092d7eb..7abc6a0705 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -680,12 +680,12 @@ AssignTransactionId(TransactionState s)
 		log_unknown_top = true;
 
 	/*
-	 * Generate a new FullTransactionId and record its xid in PG_PROC and
+	 * Generate a new FullTransactionId and record its xid in PGPROC and
 	 * pg_subtrans.
 	 *
 	 * NB: we must make the subtrans entry BEFORE the Xid appears anywhere in
-	 * shared storage other than PG_PROC; because if there's no room for it in
-	 * PG_PROC, the subtrans entry is needed to ensure that other backends see
+	 * shared storage other than PGPROC; because if there's no room for it in
+	 * PGPROC, the subtrans entry is needed to ensure that other backends see
 	 * the Xid as "running".  See GetNewTransactionId.
 	 */
 	s->fullTransactionId = GetNewTransactionId(isSubXact);
#2Kyotaro Horiguchi
horikyota.ntt@gmail.com
In reply to: Japin Li (#1)
Re: Typo in xact.c

At Thu, 15 Sep 2022 22:38:01 +0800, Japin Li <japinli@hotmail.com> wrote in

Hi hacker,

Recently, I find there might be a typo in xact.c comments. The comments
say "PG_PROC", however, it actually means "PGPROC" structure. Since we
have pg_proc catalog, and use PG_PROC to reference the catalog [1], so,
we should use PGPROC to reference the structure. Any thoughts?

[1] src/include/nodes/primnodes.h

The patch seems to me covering all occurances of PG_PROC as PGPROC.

I found several uses of PG_PROC as (pg_catalog.)pg_proc, which is
quite confusing, too..

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

#3John Naylor
john.naylor@enterprisedb.com
In reply to: Kyotaro Horiguchi (#2)
Re: Typo in xact.c

On Fri, Sep 16, 2022 at 10:11 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

At Thu, 15 Sep 2022 22:38:01 +0800, Japin Li <japinli@hotmail.com> wrote in

Hi hacker,

Recently, I find there might be a typo in xact.c comments. The comments
say "PG_PROC", however, it actually means "PGPROC" structure. Since we
have pg_proc catalog, and use PG_PROC to reference the catalog [1], so,
we should use PGPROC to reference the structure. Any thoughts?

[1] src/include/nodes/primnodes.h

The patch seems to me covering all occurances of PG_PROC as PGPROC.

+1 since this hinders grep-ability.

I found several uses of PG_PROC as (pg_catalog.)pg_proc, which is
quite confusing, too..

It's pretty obvious to me what that refers to in primnodes.h, although
the capitalization of (some, but not all) catalog names in comments in
that file is a bit strange. Maybe not worth changing there.

--
John Naylor
EDB: http://www.enterprisedb.com

#4Japin Li
japinli@hotmail.com
In reply to: John Naylor (#3)
2 attachment(s)
Re: Typo in xact.c

On Fri, 16 Sep 2022 at 11:51, John Naylor <john.naylor@enterprisedb.com> wrote:

On Fri, Sep 16, 2022 at 10:11 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

At Thu, 15 Sep 2022 22:38:01 +0800, Japin Li <japinli@hotmail.com> wrote in

Hi hacker,

Recently, I find there might be a typo in xact.c comments. The comments
say "PG_PROC", however, it actually means "PGPROC" structure. Since we
have pg_proc catalog, and use PG_PROC to reference the catalog [1], so,
we should use PGPROC to reference the structure. Any thoughts?

[1] src/include/nodes/primnodes.h

The patch seems to me covering all occurances of PG_PROC as PGPROC.

+1 since this hinders grep-ability.

I found several uses of PG_PROC as (pg_catalog.)pg_proc, which is
quite confusing, too..

It's pretty obvious to me what that refers to in primnodes.h, although
the capitalization of (some, but not all) catalog names in comments in
that file is a bit strange. Maybe not worth changing there.

Thanks for the review. I found for system catalog, we often use lower case.
Here attached a new patch to fix it.

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

Attachments:

v1-0001-Use-lower-case-to-reference-the-system-catalog.patchtext/x-patchDownload
From 1b1fd4742efe052dbca46006e72411b33a8766cd Mon Sep 17 00:00:00 2001
From: Japin Li <japinli@hotmail.com>
Date: Fri, 16 Sep 2022 11:50:38 +0800
Subject: [PATCH v1 1/2] Use lower case to reference the system catalog

---
 src/include/nodes/primnodes.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index 40661334bb..48827d291a 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -595,8 +595,8 @@ typedef enum CoercionForm
 typedef struct FuncExpr
 {
 	Expr		xpr;
-	Oid			funcid;			/* PG_PROC OID of the function */
-	Oid			funcresulttype; /* PG_TYPE OID of result value */
+	Oid			funcid;			/* pg_proc OID of the function */
+	Oid			funcresulttype; /* pg_type OID of result value */
 	bool		funcretset;		/* true if function returns set */
 	bool		funcvariadic;	/* true if variadic arguments have been
 								 * combined into an array last argument */
@@ -644,13 +644,13 @@ typedef struct OpExpr
 {
 	Expr		xpr;
 
-	/* PG_OPERATOR OID of the operator */
+	/* pg_operator OID of the operator */
 	Oid			opno;
 
-	/* PG_PROC OID of underlying function */
+	/* pg_proc OID of underlying function */
 	Oid			opfuncid pg_node_attr(equal_ignore_if_zero);
 
-	/* PG_TYPE OID of result value */
+	/* pg_type OID of result value */
 	Oid			opresulttype;
 
 	/* true if operator returns set */
@@ -721,16 +721,16 @@ typedef struct ScalarArrayOpExpr
 {
 	Expr		xpr;
 
-	/* PG_OPERATOR OID of the operator */
+	/* pg_operator OID of the operator */
 	Oid			opno;
 
-	/* PG_PROC OID of comparison function */
+	/* pg_proc OID of comparison function */
 	Oid			opfuncid pg_node_attr(equal_ignore_if_zero);
 
-	/* PG_PROC OID of hash func or InvalidOid */
+	/* pg_proc OID of hash func or InvalidOid */
 	Oid			hashfuncid pg_node_attr(equal_ignore_if_zero);
 
-	/* PG_PROC OID of negator of opfuncid function or InvalidOid.  See above */
+	/* pg_proc OID of negator of opfuncid function or InvalidOid.  See above */
 	Oid			negfuncid pg_node_attr(equal_ignore_if_zero);
 
 	/* true for ANY, false for ALL */
-- 
2.25.1

v1-0002-Fix-a-typo-about-PGPROC-structure-reference.patchtext/x-patchDownload
From 4b9ec1f791cbe865866899e8864b6e96d8fa015c Mon Sep 17 00:00:00 2001
From: Japin Li <japinli@hotmail.com>
Date: Fri, 16 Sep 2022 11:52:22 +0800
Subject: [PATCH v1 2/2] Fix a typo about PGPROC structure reference

---
 src/backend/access/transam/README | 2 +-
 src/backend/access/transam/xact.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/backend/access/transam/README b/src/backend/access/transam/README
index 734c39a4d0..72af656060 100644
--- a/src/backend/access/transam/README
+++ b/src/backend/access/transam/README
@@ -198,7 +198,7 @@ parent.  This maintains the invariant that child transactions have XIDs later
 than their parents, which is assumed in a number of places.
 
 The subsidiary actions of obtaining a lock on the XID and entering it into
-pg_subtrans and PG_PROC are done at the time it is assigned.
+pg_subtrans and PGPROC are done at the time it is assigned.
 
 A transaction that has no XID still needs to be identified for various
 purposes, notably holding locks.  For this purpose we assign a "virtual
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 50f092d7eb..7abc6a0705 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -680,12 +680,12 @@ AssignTransactionId(TransactionState s)
 		log_unknown_top = true;
 
 	/*
-	 * Generate a new FullTransactionId and record its xid in PG_PROC and
+	 * Generate a new FullTransactionId and record its xid in PGPROC and
 	 * pg_subtrans.
 	 *
 	 * NB: we must make the subtrans entry BEFORE the Xid appears anywhere in
-	 * shared storage other than PG_PROC; because if there's no room for it in
-	 * PG_PROC, the subtrans entry is needed to ensure that other backends see
+	 * shared storage other than PGPROC; because if there's no room for it in
+	 * PGPROC, the subtrans entry is needed to ensure that other backends see
 	 * the Xid as "running".  See GetNewTransactionId.
 	 */
 	s->fullTransactionId = GetNewTransactionId(isSubXact);
-- 
2.25.1

#5Japin Li
japinli@hotmail.com
In reply to: Kyotaro Horiguchi (#2)
Re: Typo in xact.c

On Fri, 16 Sep 2022 at 11:11, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

At Thu, 15 Sep 2022 22:38:01 +0800, Japin Li <japinli@hotmail.com> wrote in

Hi hacker,

Recently, I find there might be a typo in xact.c comments. The comments
say "PG_PROC", however, it actually means "PGPROC" structure. Since we
have pg_proc catalog, and use PG_PROC to reference the catalog [1], so,
we should use PGPROC to reference the structure. Any thoughts?

[1] src/include/nodes/primnodes.h

The patch seems to me covering all occurances of PG_PROC as PGPROC.

I found several uses of PG_PROC as (pg_catalog.)pg_proc, which is
quite confusing, too..

regards.

Thanks for the review. Attached a new patch to replace upper case system
catatlog to lower case [1].

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

#6John Naylor
john.naylor@enterprisedb.com
In reply to: John Naylor (#3)
Re: Typo in xact.c

On Fri, Sep 16, 2022 at 10:51 AM John Naylor
<john.naylor@enterprisedb.com> wrote:

On Fri, Sep 16, 2022 at 10:11 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

The patch seems to me covering all occurances of PG_PROC as PGPROC.

+1 since this hinders grep-ability.

Pushed this.

I found several uses of PG_PROC as (pg_catalog.)pg_proc, which is
quite confusing, too..

It's pretty obvious to me what that refers to in primnodes.h, although
the capitalization of (some, but not all) catalog names in comments in
that file is a bit strange. Maybe not worth changing there.

I left this alone. It's not wrong, and I don't think it's confusing in context.

--
John Naylor
EDB: http://www.enterprisedb.com