Re: Move global variables of pgoutput to plugin private scope.

2023-09-28 Thread Michael Paquier
On Wed, Sep 27, 2023 at 04:57:06PM +0900, Michael Paquier wrote: > Sure. I found the concept behind 0002 sound. Feel free to go ahead > with 0001, and I can always look at the second. Always happy to help. For the sake of the archives: - Amit has applied 0001 down to 16 as of 54ccfd65868c. -

Re: Move global variables of pgoutput to plugin private scope.

2023-09-27 Thread Michael Paquier
On Wed, Sep 27, 2023 at 10:51:52AM +0530, Amit Kapila wrote: > I have briefly looked at > v2-0002-Move-in_streaming-to-output-private-data in the same email [1] > but didn't think about it in detail (like whether there is any live > bug that can be fixed or is just an improvement). This looks

Re: Move global variables of pgoutput to plugin private scope.

2023-09-26 Thread Michael Paquier
On Wed, Sep 27, 2023 at 04:51:29AM +, Zhijie Hou (Fujitsu) wrote: > While searching the code, I noticed one postgres fork where the PGoutputData > is > used in other places, although it's a separate fork, but it seems better to > discuss the removal separately. > > [1] >

Re: Move global variables of pgoutput to plugin private scope.

2023-09-26 Thread Amit Kapila
On Wed, Sep 27, 2023 at 10:26 AM Michael Paquier wrote: > > On Wed, Sep 27, 2023 at 10:15:24AM +0530, Amit Kapila wrote: > > It's like that from the beginning. Now, even if we want to move, your > > suggestion is not directly related to this patch as we are just > > changing one field, and that

Re: Move global variables of pgoutput to plugin private scope.

2023-09-26 Thread Michael Paquier
On Wed, Sep 27, 2023 at 10:15:24AM +0530, Amit Kapila wrote: > It's like that from the beginning. Now, even if we want to move, your > suggestion is not directly related to this patch as we are just > changing one field, and that too to fix a bug. We should start a > separate thread to gather a

RE: Move global variables of pgoutput to plugin private scope.

2023-09-26 Thread Zhijie Hou (Fujitsu)
On Wednesday, September 27, 2023 12:45 PM Amit Kapila > > On Wed, Sep 27, 2023 at 9:46 AM Michael Paquier > wrote: > > > > On Wed, Sep 27, 2023 at 09:39:19AM +0530, Amit Kapila wrote: > > > On Wed, Sep 27, 2023 at 9:10 AM Michael Paquier > wrote: > > >> Err, actually, I am going to disagree

Re: Move global variables of pgoutput to plugin private scope.

2023-09-26 Thread Amit Kapila
On Wed, Sep 27, 2023 at 9:46 AM Michael Paquier wrote: > > On Wed, Sep 27, 2023 at 09:39:19AM +0530, Amit Kapila wrote: > > On Wed, Sep 27, 2023 at 9:10 AM Michael Paquier wrote: > >> Err, actually, I am going to disagree here for the patch of HEAD. It > >> seems to me that there is zero need

Re: Move global variables of pgoutput to plugin private scope.

2023-09-26 Thread Michael Paquier
On Wed, Sep 27, 2023 at 09:39:19AM +0530, Amit Kapila wrote: > On Wed, Sep 27, 2023 at 9:10 AM Michael Paquier wrote: >> Err, actually, I am going to disagree here for the patch of HEAD. It >> seems to me that there is zero need for pgoutput.h and we don't need >> to show PGOutputData to the

Re: Move global variables of pgoutput to plugin private scope.

2023-09-26 Thread Amit Kapila
On Wed, Sep 27, 2023 at 9:10 AM Michael Paquier wrote: > > On Tue, Sep 26, 2023 at 01:55:10PM +, Zhijie Hou (Fujitsu) wrote: > > On Tuesday, September 26, 2023 4:40 PM Amit Kapila > > wrote: > >> Do we really need a new parameter in above structure? Can't we just use the > >> existing

Re: Move global variables of pgoutput to plugin private scope.

2023-09-26 Thread Michael Paquier
On Tue, Sep 26, 2023 at 01:55:10PM +, Zhijie Hou (Fujitsu) wrote: > On Tuesday, September 26, 2023 4:40 PM Amit Kapila > wrote: >> Do we really need a new parameter in above structure? Can't we just use the >> existing origin in the same structure? Please remember if this needs to be >>

RE: Move global variables of pgoutput to plugin private scope.

2023-09-26 Thread Zhijie Hou (Fujitsu)
On Tuesday, September 26, 2023 4:40 PM Amit Kapila wrote: > > On Tue, Sep 19, 2023 at 12:48 PM Zhijie Hou (Fujitsu) > wrote: > > > > - static bool publish_no_origin; > > > > This flag is also local to pgoutput instance, and we didn't reset the > > flag in output shutdown callback, so if we

RE: Move global variables of pgoutput to plugin private scope.

2023-09-26 Thread Zhijie Hou (Fujitsu)
On Tuesday, September 19, 2023 1:44 PM Michael Paquier wrote: > > On Tue, Sep 19, 2023 at 04:10:39AM +, Zhijie Hou (Fujitsu) wrote: > > Currently we have serval global variables in pgoutput, but each of > > them is inherently local to an individual pgoutput instance. This > > could cause

Re: Move global variables of pgoutput to plugin private scope.

2023-09-26 Thread Amit Kapila
On Tue, Sep 19, 2023 at 12:48 PM Zhijie Hou (Fujitsu) wrote: > > - static bool publish_no_origin; > > This flag is also local to pgoutput instance, and we didn't reset the flag in > output shutdown callback, so if we consume changes from different slots, then > the second call would reuse the

Re: Move global variables of pgoutput to plugin private scope.

2023-09-19 Thread Peter Smith
Hi Hou-san. Given there are some issues raised about the 0001 patch [1] I am skipping that one until I see the replies. Meanwhile, here are some review comments for the patches v1-0002 and v1-0003 v1-0002 == Commit message 1. The pgoutput module uses a global

Re: Move global variables of pgoutput to plugin private scope.

2023-09-18 Thread Michael Paquier
On Tue, Sep 19, 2023 at 04:10:39AM +, Zhijie Hou (Fujitsu) wrote: > Currently we have serval global variables in pgoutput, but each of them is > inherently local to an individual pgoutput instance. This could cause issues > if > we switch to different output plugin instance in one session and