Re: [HACKERS] Subscription code improvements
I have committed the patches 0001 Improve messaging during logical replication worker startup 0002 Split the SetSubscriptionRelState function into two 0004 doesn't apply anymore, 0003 doesn't seem to be very useful without 0004, as I understand it. It's also a bit more than I'm comfortable reviewing or committing right now. So maybe move those to the next commitfest or close the entry as returned with feedback? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] Subscription code improvements
On Wed, Mar 7, 2018 at 11:13 PM, Alvaro Herrera wrote: > 0002 looks like a good improvement to me. The existing routine is > messy, and apparently it's so just to save one LockSharedObject plus > cache lookup; IMO it's not worth it. Patched code looks simpler. If > there are cases where having the combined behavior is useful, it's not > clear what they are. (If I understand correctly, the reason is that a > sync worker could try to insert-or-update the row after some other > process deleted it [because of removing the table from subscription?] > ... but that seems to work out *simpler* with the new code. So what's > up?) > The function calling SetSubscriptionRelState with update_only=false (i.g. going to do insert-or-update) is two function: CreateSubscription() and AlterSubscription_refresh(). AFAICS these two function actually doesn't need such insert-or-update functionality because it doesn't happen that a backend process creates/alters the same name subscription which already exists. Since CreateSubscirption() inserts a heap into the system catalog one transaction ends up with the error of key already exists if two process tries to create the same name subscription . Similarly for AlterSubscription_refresh(), since we acquire the AccessExclusiveLock on the subscription object before getting the new table list in the publication the updating a existing entry doesn't happen. So this patch changes SetsubscriptionRelState() with update_only=fasle to AddSubscriptionRelState() and others to UpdateSubscriptionRelState(). Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Re: [HACKERS] Subscription code improvements
On Wed, Mar 7, 2018 at 11:13 PM, Alvaro Herrera wrote:
> 0001:
>
> there are a bunch of other messages of the same ilk in the file. I
> don't like how the current messages are worded; maybe Peter or Petr had
> some reason why they're like that, but I would have split out the reason
> for not starting or stopping into errdetail. Something like
>
> errmsg("logical replication apply worker for subscription \"%s\" will not
> start", ...)
> errdetail("Subscription has been disabled during startup.")
>
> But I think we should change all these messages in unison rather than
> only one of them.
>
> Now, your patch changes "will not start" to "will stop". But is that
> correct? ISTM that this happens when a worker is starting and
> determines that it is not needed. So "will not start" is more correct.
> "Will stop" would be correct if the worker had been running for some
> time and suddenly decided to terminate, but that doesn't seem to be the
> case, unless I misread the code.
>
> Your patch also changes "disabled during startup" to just "disabled".
> Is that a useful change? ISTM that if the subscription had been
> disabled prior to startup, then the worker would not have started at
> all, so we would not be seeing this message in the first place. Again,
> maybe I am misreading the code? Please explain.
I think you're not misreading the code. I agree with you. "will not
start" and "disabled during startup" would be more appropriate here.
But Petr might have other opinion on this. Also I noticed I overlooked
one change of v1 patch proposed by Petr. Attached a new patch
incorporated your review comment and the hunk I overlooked.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
0001-Improve-messaging-during-logical-replication-worker-_v3.patch
Description: Binary data
Re: [HACKERS] Subscription code improvements
On 3/7/18 9:37 AM, Alvaro Herrera wrote: > David Steele wrote: > >> I'm marking this submission Returned with Feedback. > > Not yet please. Back to Waiting on Author state. Regards, -- -David da...@pgmasters.net
Re: [HACKERS] Subscription code improvements
David Steele wrote: > I'm marking this submission Returned with Feedback. Not yet please. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] Subscription code improvements
On 3/7/18 7:41 AM, Masahiko Sawada wrote: > On Tue, Mar 6, 2018 at 11:17 PM, David Steele wrote: >> Hi Masahiko, >> >> On 1/30/18 5:00 AM, Masahiko Sawada wrote: >>> On Fri, Jan 26, 2018 at 11:41 AM, Peter Eisentraut >>> wrote: On 1/24/18 02:33, Masahiko Sawada wrote: > Thank you for notification. Since it seems to me that no one is > interested in this patch, it would be better to close out this patch. > This patch is a refactoring patch but subscription code seems to work > fine so far. If a problem appears around subscriptions, I might > propose it again. >> >> This patch is still in the Waiting for Author state but it looks like >> you intended to close it. Should I do so now? > > Uh, actually three(0001 - 0002) of four patches applies cleanly to > current HEAD, and I think we can regards them as "Needs Review". Only > 0003 and 0004 patch are under "Waiting on Author". Since 0001 and 0002 > are very simple I hope these patch get reviewed. It was a my fault to > get different patches into one CF entry. I rather doubt you are going to attract any review as long is this stays in Waiting on Author state -- which I notice it has been in since the end of the November CF. That is reason enough to return it since we have been taking a pretty firm stance on patches that have not been updated for the CF. I'm marking this submission Returned with Feedback. Regards, -- -David da...@pgmasters.net
Re: [HACKERS] Subscription code improvements
0002 looks like a good improvement to me. The existing routine is messy, and apparently it's so just to save one LockSharedObject plus cache lookup; IMO it's not worth it. Patched code looks simpler. If there are cases where having the combined behavior is useful, it's not clear what they are. (If I understand correctly, the reason is that a sync worker could try to insert-or-update the row after some other process deleted it [because of removing the table from subscription?] ... but that seems to work out *simpler* with the new code. So what's up?) -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] Subscription code improvements
0001:
there are a bunch of other messages of the same ilk in the file. I
don't like how the current messages are worded; maybe Peter or Petr had
some reason why they're like that, but I would have split out the reason
for not starting or stopping into errdetail. Something like
errmsg("logical replication apply worker for subscription \"%s\" will not
start", ...)
errdetail("Subscription has been disabled during startup.")
But I think we should change all these messages in unison rather than
only one of them.
Now, your patch changes "will not start" to "will stop". But is that
correct? ISTM that this happens when a worker is starting and
determines that it is not needed. So "will not start" is more correct.
"Will stop" would be correct if the worker had been running for some
time and suddenly decided to terminate, but that doesn't seem to be the
case, unless I misread the code.
Your patch also changes "disabled during startup" to just "disabled".
Is that a useful change? ISTM that if the subscription had been
disabled prior to startup, then the worker would not have started at
all, so we would not be seeing this message in the first place. Again,
maybe I am misreading the code? Please explain.
--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Re: [HACKERS] Subscription code improvements
On Tue, Mar 6, 2018 at 11:17 PM, David Steele wrote: > Hi Masahiko, > > On 1/30/18 5:00 AM, Masahiko Sawada wrote: >> On Fri, Jan 26, 2018 at 11:41 AM, Peter Eisentraut >> wrote: >>> On 1/24/18 02:33, Masahiko Sawada wrote: Thank you for notification. Since it seems to me that no one is interested in this patch, it would be better to close out this patch. This patch is a refactoring patch but subscription code seems to work fine so far. If a problem appears around subscriptions, I might propose it again. > > This patch is still in the Waiting for Author state but it looks like > you intended to close it. Should I do so now? > Uh, actually three(0001 - 0002) of four patches applies cleanly to current HEAD, and I think we can regards them as "Needs Review". Only 0003 and 0004 patch are under "Waiting on Author". Since 0001 and 0002 are very simple I hope these patch get reviewed. It was a my fault to get different patches into one CF entry. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Re: Re: [HACKERS] Subscription code improvements
Hi Masahiko, On 1/30/18 5:00 AM, Masahiko Sawada wrote: > On Fri, Jan 26, 2018 at 11:41 AM, Peter Eisentraut > wrote: >> On 1/24/18 02:33, Masahiko Sawada wrote: >>> Thank you for notification. Since it seems to me that no one is >>> interested in this patch, it would be better to close out this patch. >>> This patch is a refactoring patch but subscription code seems to work >>> fine so far. If a problem appears around subscriptions, I might >>> propose it again. This patch is still in the Waiting for Author state but it looks like you intended to close it. Should I do so now? Thanks, -- -David da...@pgmasters.net
Re: [HACKERS] Subscription code improvements
On Fri, Jan 26, 2018 at 11:41 AM, Peter Eisentraut wrote: > On 1/24/18 02:33, Masahiko Sawada wrote: >> Thank you for notification. Since it seems to me that no one is >> interested in this patch, it would be better to close out this patch. >> This patch is a refactoring patch but subscription code seems to work >> fine so far. If a problem appears around subscriptions, I might >> propose it again. > > I have looked at the patches again. Thank you for looking at this. > They seem generally reasonable, but > I don't see quite why we want or need them. More details would help > review them. Do they fix any bugs, or are they just rearranging code? > 0002 patch rearranges the code. Currently SetSubscriptionRelState() not only update but also register a record, and it is controlled by update_only argument. This patch splits SetSubscriptionRelState() into AddSubscriptionRelState() and UpdateSubscriptionRelstate(). 0001, 0003 and 0004 patch are improvement patches. 0001 patch improves messaging during worker startup. 0004 patch, which requires 0003 patch, patch reduce the lock level for DROP SUBSCRIPTION to RowExclusiveLock. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Re: [HACKERS] Subscription code improvements
On 1/24/18 02:33, Masahiko Sawada wrote: > Thank you for notification. Since it seems to me that no one is > interested in this patch, it would be better to close out this patch. > This patch is a refactoring patch but subscription code seems to work > fine so far. If a problem appears around subscriptions, I might > propose it again. I have looked at the patches again. They seem generally reasonable, but I don't see quite why we want or need them. More details would help review them. Do they fix any bugs, or are they just rearranging code? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] Subscription code improvements
On Tue, Jan 23, 2018 at 7:58 AM, Stephen Frost wrote: > Greetings, > > * Michael Paquier (michael.paqu...@gmail.com) wrote: >> On Fri, Aug 18, 2017 at 2:09 PM, Masahiko Sawada >> wrote: >> > On Thu, Aug 17, 2017 at 8:17 PM, Masahiko Sawada >> > wrote: >> >> On Thu, Aug 17, 2017 at 9:10 AM, Peter Eisentraut >> >> wrote: >> >>> On 8/8/17 05:58, Masahiko Sawada wrote: >> Are you planning to work on remaining patches 0005 and 0006 that >> improve the subscription codes in PG11 cycle? If not, I will take over >> them and work on the next CF. >> >>> >> >>> Based on your assessment, the remaining patches were not required bug >> >>> fixes. So I think preparing them for the next commit fest would be >> >>> great. >> >>> >> >> >> >> Thank you for the comment. >> >> >> >> After more thought, since 0001 and 0003 patches on the first mail also >> >> improve the subscription codes and are worth to be considered, I >> >> picked total 4 patches up and updated them. I'm planning to work on >> >> these patches in the next CF. >> >> >> > >> > Added this item to the next commit fest. >> >> The patch set fails to apply. Please provide a rebased version. I am >> moving this entry to next CF with waiting on author as status. > > Masahiko Sawada, this patch is still in Waiting on Author and hasn't > progressed in a very long time. Is there any chance you'll be able to > provide an updated patch soon for review? Or should this patch be > closed out? > Thank you for notification. Since it seems to me that no one is interested in this patch, it would be better to close out this patch. This patch is a refactoring patch but subscription code seems to work fine so far. If a problem appears around subscriptions, I might propose it again. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Re: [HACKERS] Subscription code improvements
Greetings, * Michael Paquier (michael.paqu...@gmail.com) wrote: > On Fri, Aug 18, 2017 at 2:09 PM, Masahiko Sawada > wrote: > > On Thu, Aug 17, 2017 at 8:17 PM, Masahiko Sawada > > wrote: > >> On Thu, Aug 17, 2017 at 9:10 AM, Peter Eisentraut > >> wrote: > >>> On 8/8/17 05:58, Masahiko Sawada wrote: > Are you planning to work on remaining patches 0005 and 0006 that > improve the subscription codes in PG11 cycle? If not, I will take over > them and work on the next CF. > >>> > >>> Based on your assessment, the remaining patches were not required bug > >>> fixes. So I think preparing them for the next commit fest would be great. > >>> > >> > >> Thank you for the comment. > >> > >> After more thought, since 0001 and 0003 patches on the first mail also > >> improve the subscription codes and are worth to be considered, I > >> picked total 4 patches up and updated them. I'm planning to work on > >> these patches in the next CF. > >> > > > > Added this item to the next commit fest. > > The patch set fails to apply. Please provide a rebased version. I am > moving this entry to next CF with waiting on author as status. Masahiko Sawada, this patch is still in Waiting on Author and hasn't progressed in a very long time. Is there any chance you'll be able to provide an updated patch soon for review? Or should this patch be closed out? Thanks! Stephen signature.asc Description: PGP signature
Re: [HACKERS] Subscription code improvements
On Fri, Aug 18, 2017 at 2:09 PM, Masahiko Sawada wrote: > On Thu, Aug 17, 2017 at 8:17 PM, Masahiko Sawada > wrote: >> On Thu, Aug 17, 2017 at 9:10 AM, Peter Eisentraut >> wrote: >>> On 8/8/17 05:58, Masahiko Sawada wrote: Are you planning to work on remaining patches 0005 and 0006 that improve the subscription codes in PG11 cycle? If not, I will take over them and work on the next CF. >>> >>> Based on your assessment, the remaining patches were not required bug >>> fixes. So I think preparing them for the next commit fest would be great. >>> >> >> Thank you for the comment. >> >> After more thought, since 0001 and 0003 patches on the first mail also >> improve the subscription codes and are worth to be considered, I >> picked total 4 patches up and updated them. I'm planning to work on >> these patches in the next CF. >> > > Added this item to the next commit fest. The patch set fails to apply. Please provide a rebased version. I am moving this entry to next CF with waiting on author as status. -- Michael
