Обсуждение: Functions 'is_publishable_class' and 'is_publishable_relation' should stay together.

During a recent review, I happened to notice that in the file
src/backend/catalog/pg_publication.c the two functions
'is_publishable_class' and 'is_publishable_relation' used to be [1]
adjacent in the source code. This is also evident in
'is_publishable_relation' because the wording of the function comment
just refers to the prior function (e.g. "Another variant of this,
taking a Relation.") and also this just "wraps" the prior function.

It seems that sometime last year another commit [2] inadvertently
inserted another function ('filter_partitions') between those
aforementioned, and that means the "Another variant of this" comment
doesn't make much sense anymore.

PSA a patch just to put those original 2 functions back together
again. No code is "changed" - only moved.

------

[1]
https://github.com/postgres/postgres/blame/f0b051e322d530a340e62f2ae16d99acdbcb3d05/src/backend/catalog/pg_publication.c
[2]
https://github.com/postgres/postgres/commit/5a2832465fd8984d089e8c44c094e6900d987fcd#diff-1ecc273c7808aba21749ea2718482c153cd6c4dc9d90c69124f3a7c5963b2b4a

Kind Regards,
Peter Smith.
Fujitsu Australia

On Friday, July 29, 2022 7:17 AM Peter Smith <smithpb2250@gmail.com> wrote:
> During a recent review, I happened to notice that in the file
> src/backend/catalog/pg_publication.c the two functions 'is_publishable_class'
> and 'is_publishable_relation' used to be [1] adjacent in the source code. This is
> also evident in 'is_publishable_relation' because the wording of the function
> comment just refers to the prior function (e.g. "Another variant of this, taking a
> Relation.") and also this just "wraps" the prior function.
> 
> It seems that sometime last year another commit [2] inadvertently inserted
> another function ('filter_partitions') between those aforementioned, and that
> means the "Another variant of this" comment doesn't make much sense
> anymore.

Agreed.

Personally, I think it would be better to modify the comments of
is_publishable_relation and directly mention the function name it refers to
which can prevent future code to break it again.

Besides,

/*
 * Returns if relation represented by oid and Form_pg_class entry
 * is publishable.
 *
 * Does same checks as the above,

This comment was also intended to refer to the function
check_publication_add_relation(), but is invalid now because there is another
function check_publication_add_schema() inserted between them. We'd better fix
this as well.

Best regards,
Hou zj

On Fri, Jul 29, 2022 at 11:55 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Friday, July 29, 2022 7:17 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > During a recent review, I happened to notice that in the file
> > src/backend/catalog/pg_publication.c the two functions 'is_publishable_class'
> > and 'is_publishable_relation' used to be [1] adjacent in the source code. This is
> > also evident in 'is_publishable_relation' because the wording of the function
> > comment just refers to the prior function (e.g. "Another variant of this, taking a
> > Relation.") and also this just "wraps" the prior function.
> >
> > It seems that sometime last year another commit [2] inadvertently inserted
> > another function ('filter_partitions') between those aforementioned, and that
> > means the "Another variant of this" comment doesn't make much sense
> > anymore.
>
> Agreed.
>
> Personally, I think it would be better to modify the comments of
> is_publishable_relation and directly mention the function name it refers to
> which can prevent future code to break it again.

I'd intended only to make the minimal changes necessary to set things
right again, but your way is better.

>
> Besides,
>
> /*
>  * Returns if relation represented by oid and Form_pg_class entry
>  * is publishable.
>  *
>  * Does same checks as the above,
>
> This comment was also intended to refer to the function
> check_publication_add_relation(), but is invalid now because there is another
> function check_publication_add_schema() inserted between them. We'd better fix
> this as well.

Thanks, I'll post another patch later to address that one too.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, Jul 29, 2022 at 8:26 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Fri, Jul 29, 2022 at 11:55 AM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Friday, July 29, 2022 7:17 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > > During a recent review, I happened to notice that in the file
> > > src/backend/catalog/pg_publication.c the two functions 'is_publishable_class'
> > > and 'is_publishable_relation' used to be [1] adjacent in the source code. This is
> > > also evident in 'is_publishable_relation' because the wording of the function
> > > comment just refers to the prior function (e.g. "Another variant of this, taking a
> > > Relation.") and also this just "wraps" the prior function.
> > >
> > > It seems that sometime last year another commit [2] inadvertently inserted
> > > another function ('filter_partitions') between those aforementioned, and that
> > > means the "Another variant of this" comment doesn't make much sense
> > > anymore.
> >
> > Agreed.
> >
> > Personally, I think it would be better to modify the comments of
> > is_publishable_relation and directly mention the function name it refers to
> > which can prevent future code to break it again.
>
> I'd intended only to make the minimal changes necessary to set things
> right again, but your way is better.
>

Yeah, Hou-San's suggestion sounds better to me as well.

> >
> > Besides,
> >
> > /*
> >  * Returns if relation represented by oid and Form_pg_class entry
> >  * is publishable.
> >  *
> >  * Does same checks as the above,
> >
> > This comment was also intended to refer to the function
> > check_publication_add_relation(), but is invalid now because there is another
> > function check_publication_add_schema() inserted between them. We'd better fix
> > this as well.
>

+1. Here, I think it will be better to add the function name in the
comments and keep the current order as it is.


-- 
With Regards,
Amit Kapila.

PSA v2 of this patch, modified as suggested.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On 2022-Jul-29, Peter Smith wrote:

> PSA v2 of this patch, modified as suggested.

I don't object to doing this, but I think these two functions should
stay together nonetheless.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
Y una voz del caos me habló y me dijo
"Sonríe y sé feliz, podría ser peor".
Y sonreí. Y fui feliz.
Y fue peor.

On Fri, Jul 29, 2022 at 7:36 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2022-Jul-29, Peter Smith wrote:
>
> > PSA v2 of this patch, modified as suggested.
>
> I don't object to doing this, but I think these two functions should
> stay together nonetheless.


Hmm, I think there is some confusion because different people have
mentioned multiple functions.

AFAIK, the patch *does* ensure the 2 functions (is_publishable_class
and is_publishable_relation) stay together.

If you believe there is still a problem after applying the patch
please explicitly name what function(s) you think should be moved.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On 2022-Jul-29, Peter Smith wrote:

> On Fri, Jul 29, 2022 at 7:36 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > I don't object to doing this, but I think these two functions should
> > stay together nonetheless.
> 
> If you believe there is still a problem after applying the patch
> please explicitly name what function(s) you think should be moved.

Well, I checked the commit and the functions I was talking about look OK
now.  However, looking again, pg_relation_is_publishable is in the wrong
place (should be right below is_publishable_relaton), and I wonder why
aren't get_publication_oid and get_publication_name in lsyscache.c.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

On Fri, Jul 29, 2022 at 3:29 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2022-Jul-29, Peter Smith wrote:
>
> > On Fri, Jul 29, 2022 at 7:36 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > >
> > > I don't object to doing this, but I think these two functions should
> > > stay together nonetheless.
> >
> > If you believe there is still a problem after applying the patch
> > please explicitly name what function(s) you think should be moved.
>
> Well, I checked the commit and the functions I was talking about look OK
> now.  However, looking again, pg_relation_is_publishable is in the wrong
> place (should be right below is_publishable_relaton), and I wonder why
> aren't get_publication_oid and get_publication_name in lsyscache.c.
>

Right, both these suggestions make sense to me. Similarly, I think
functions get_subscription_name and get_subscription_oid should also
be moved to lsyscache.c.

-- 
With Regards,
Amit Kapila.

On Fri, Jul 29, 2022 at 3:55 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Jul 29, 2022 at 3:29 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > Well, I checked the commit and the functions I was talking about look OK
> > now.  However, looking again, pg_relation_is_publishable is in the wrong
> > place (should be right below is_publishable_relaton), and I wonder why
> > aren't get_publication_oid and get_publication_name in lsyscache.c.
> >
>
> Right, both these suggestions make sense to me. Similarly, I think
> functions get_subscription_name and get_subscription_oid should also
> be moved to lsyscache.c.
>

Attached, find a patch to address the above comments.

Note that (a) I didn't change the comment atop
pg_relation_is_publishable to refer to the actual function name
instead of 'above' as it seems it can be an SQL variant for both the
above functions. (b) didn't need to include pg_publication.h in
lsyscache.c even after moving code to that file as the code is
compiled even without that.


-- 
With Regards,
Amit Kapila.

On Saturday, July 30, 2022 7:25 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Fri, Jul 29, 2022 at 3:55 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Jul 29, 2022 at 3:29 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
> wrote:
> > >
> > > Well, I checked the commit and the functions I was talking about
> > > look OK now.  However, looking again, pg_relation_is_publishable is
> > > in the wrong place (should be right below is_publishable_relaton),
> > > and I wonder why aren't get_publication_oid and get_publication_name in
> lsyscache.c.
> > >
> >
> > Right, both these suggestions make sense to me. Similarly, I think
> > functions get_subscription_name and get_subscription_oid should also
> > be moved to lsyscache.c.
> >
> 
> Attached, find a patch to address the above comments.
> 
> Note that (a) I didn't change the comment atop pg_relation_is_publishable to
> refer to the actual function name instead of 'above' as it seems it can be an SQL
> variant for both the above functions. (b) didn't need to include pg_publication.h
> in lsyscache.c even after moving code to that file as the code is compiled even
> without that.

The patch LGTM. I also ran the headerscheck and didn't find any problem.

Best regards,
Hou Zhijie

On Sat, Jul 30, 2022 at 6:59 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Saturday, July 30, 2022 7:25 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Jul 29, 2022 at 3:55 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Fri, Jul 29, 2022 at 3:29 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
> > wrote:
> > > >
> > > > Well, I checked the commit and the functions I was talking about
> > > > look OK now.  However, looking again, pg_relation_is_publishable is
> > > > in the wrong place (should be right below is_publishable_relaton),
> > > > and I wonder why aren't get_publication_oid and get_publication_name in
> > lsyscache.c.
> > > >
> > >
> > > Right, both these suggestions make sense to me. Similarly, I think
> > > functions get_subscription_name and get_subscription_oid should also
> > > be moved to lsyscache.c.
> > >
> >
> > Attached, find a patch to address the above comments.
> >
> > Note that (a) I didn't change the comment atop pg_relation_is_publishable to
> > refer to the actual function name instead of 'above' as it seems it can be an SQL
> > variant for both the above functions. (b) didn't need to include pg_publication.h
> > in lsyscache.c even after moving code to that file as the code is compiled even
> > without that.
>
> The patch LGTM. I also ran the headerscheck and didn't find any problem.
>

Thanks, I have pushed the patch.

-- 
With Regards,
Amit Kapila.