Thread: pg_stat_get_replication_slot and pg_stat_get_subscription_worker incorrectly marked as proretset

Hi all,
(Author and committer added in CC.)

While reviewing the code of a bunch of SRF functions in the core code,
I have noticed that the two functions mentioned in $subject are marked
as proretset but both functions don't return a set of tuples, just one
record for the object given in input.  It is also worth noting that
prorows is set to 1.

This looks like a copy-pasto error that has spread around.  The error
on pg_stat_get_subscription_worker is recent as of 8d74fc9, and the
one on pg_stat_get_replication_slot has been introduced in 3fa17d3,
meaning that REL_14_STABLE got it wrong for the second part.

I am aware about the discussions on the parent view for the first
case and its design issues, but it does not change the fact that we'd
better address the second case on HEAD IMO.

Thoughts?
--
Michael

Hi,

On Mon, Feb 21, 2022 at 2:36 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> Hi all,
> (Author and committer added in CC.)
>
> While reviewing the code of a bunch of SRF functions in the core code,
> I have noticed that the two functions mentioned in $subject are marked
> as proretset but both functions don't return a set of tuples, just one
> record for the object given in input.  It is also worth noting that
> prorows is set to 1.

Thanks for pointing it out. Agreed.

>
> This looks like a copy-pasto error that has spread around.  The error
> on pg_stat_get_subscription_worker is recent as of 8d74fc9, and the
> one on pg_stat_get_replication_slot has been introduced in 3fa17d3,
> meaning that REL_14_STABLE got it wrong for the second part.
>
> I am aware about the discussions on the parent view for the first
> case and its design issues, but it does not change the fact that we'd
> better address the second case on HEAD IMO.
>
> Thoughts?

Agreed.

Regards,


--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On Mon, Feb 21, 2022 at 11:21 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> >
> > I am aware about the discussions on the parent view for the first
> > case and its design issues, but it does not change the fact that we'd
> > better address the second case on HEAD IMO.
> >
> > Thoughts?
>
> Agreed.
>

+1. How about attached?

-- 
With Regards,
Amit Kapila.

On Mon, Feb 21, 2022 at 04:19:59PM +0530, Amit Kapila wrote:
> On Mon, Feb 21, 2022 at 11:21 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>> Agreed.
>>
>
> +1. How about attached?

That's the same thing as what I sent upthread, so that's correct to
me, except that I have fixed both functions :)

You are not touching pg_stat_get_subscription_worker() because the
plan is to revert it from HEAD?  I have not followed the other
discussion closely.
--
Michael

On Mon, Feb 21, 2022 at 4:56 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Feb 21, 2022 at 04:19:59PM +0530, Amit Kapila wrote:
> > On Mon, Feb 21, 2022 at 11:21 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >> Agreed.
> >>
> >
> > +1. How about attached?
>
> That's the same thing as what I sent upthread, so that's correct to
> me, except that I have fixed both functions :)
>

Sorry, I hadn't looked at your patch.

> You are not touching pg_stat_get_subscription_worker() because the
> plan is to revert it from HEAD?  I have not followed the other
> discussion closely.
>

It is still under discussion. I'll take the necessary action along
with other things related to that view based on the conclusion on that
thread.

-- 
With Regards,
Amit Kapila.

On Mon, Feb 21, 2022 at 05:00:43PM +0530, Amit Kapila wrote:
> On Mon, Feb 21, 2022 at 4:56 PM Michael Paquier <michael@paquier.xyz> wrote:
>> That's the same thing as what I sent upthread, so that's correct to
>> me, except that I have fixed both functions :)
>
> Sorry, I hadn't looked at your patch.

That's fine.  This is something you have committed, after all.

>> You are not touching pg_stat_get_subscription_worker() because the
>> plan is to revert it from HEAD?  I have not followed the other
>> discussion closely.
>>
>
> It is still under discussion. I'll take the necessary action along
> with other things related to that view based on the conclusion on that
> thread.

Sounds good to me.  The patch you are proposing upthread is OK for
me.
--
Michael

On Tue, Feb 22, 2022 at 8:15 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Feb 21, 2022 at 05:00:43PM +0530, Amit Kapila wrote:
> > On Mon, Feb 21, 2022 at 4:56 PM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > It is still under discussion. I'll take the necessary action along
> > with other things related to that view based on the conclusion on that
> > thread.
>
> Sounds good to me.  The patch you are proposing upthread is OK for
> me.
>

Thanks, so you are okay with me pushing that patch just to HEAD. We
don't want to backpatch this to 14 as this is a catalog change and
won't cause any user-visible issue, is that correct?

-- 
With Regards,
Amit Kapila.

On Wed, Feb 23, 2022 at 09:52:02AM +0530, Amit Kapila wrote:
> Thanks, so you are okay with me pushing that patch just to HEAD.

Yes, I am fine with that.  I am wondering about patching the second
function though, to avoid any risk of forgetting it, but I am fine to
leave that to your judgement.

> We don't want to backpatch this to 14 as this is a catalog change and
> won't cause any user-visible issue, is that correct?

Yup, that's a HEAD-only cleanup, I am afraid.
--
Michael

On Thu, Feb 24, 2022 at 12:03 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Feb 23, 2022 at 09:52:02AM +0530, Amit Kapila wrote:
> > Thanks, so you are okay with me pushing that patch just to HEAD.
>
> Yes, I am fine with that.  I am wondering about patching the second
> function though, to avoid any risk of forgetting it, but I am fine to
> leave that to your judgement.
>

The corresponding patch with other changes is not very far from being
ready to commit. So, will do it along with that.

> > We don't want to backpatch this to 14 as this is a catalog change and
> > won't cause any user-visible issue, is that correct?
>
> Yup, that's a HEAD-only cleanup, I am afraid.
>

Thanks, Done!

-- 
With Regards,
Amit Kapila.

On Fri, Feb 25, 2022 at 8:57 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Feb 24, 2022 at 12:03 PM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Wed, Feb 23, 2022 at 09:52:02AM +0530, Amit Kapila wrote:
> > > Thanks, so you are okay with me pushing that patch just to HEAD.
> >
> > Yes, I am fine with that.  I am wondering about patching the second
> > function though, to avoid any risk of forgetting it, but I am fine to
> > leave that to your judgement.
> >
>
> The corresponding patch with other changes is not very far from being
> ready to commit. So, will do it along with that.
>

This is done as part of commit:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=7a85073290856554416353a89799a4c04d09b74b


-- 
With Regards,
Amit Kapila.

On Wed, Mar 02, 2022 at 07:42:50AM +0530, Amit Kapila wrote:
> This is done as part of commit:
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=7a85073290856554416353a89799a4c04d09b74b

Thanks for taking care of it!
--
Michael