Обсуждение: Typo in xact.c

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

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

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);

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

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

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.

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.

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