Обсуждение: pg_stat_get_replication_slot and pg_stat_get_subscription_worker incorrectly marked as proretset
pg_stat_get_replication_slot and pg_stat_get_subscription_worker incorrectly marked as proretset
От
Michael Paquier
Дата:
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
Вложения
Re: pg_stat_get_replication_slot and pg_stat_get_subscription_worker incorrectly marked as proretset
От
Masahiko Sawada
Дата:
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.
Вложения
Re: pg_stat_get_replication_slot and pg_stat_get_subscription_worker incorrectly marked as proretset
От
Michael Paquier
Дата:
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.
Re: pg_stat_get_replication_slot and pg_stat_get_subscription_worker incorrectly marked as proretset
От
Michael Paquier
Дата:
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.
Re: pg_stat_get_replication_slot and pg_stat_get_subscription_worker incorrectly marked as proretset
От
Michael Paquier
Дата:
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.
Re: pg_stat_get_replication_slot and pg_stat_get_subscription_worker incorrectly marked as proretset
От
Michael Paquier
Дата:
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