On Thu, Jul 8, 2021 at 6:24 AM Peter Smith wrote:
> OK. I created a new thread called "parse_subscription_options -
> suggested improvements" here [1]
Thanks. I closed the CF entry for this thread.
Regards,
Bharath Rupireddy.
On Wed, Jul 7, 2021 at 1:35 PM Bharath Rupireddy
wrote:
>
> On Wed, Jul 7, 2021 at 5:33 AM Peter Smith wrote:
> > PSA my patch which includes all the fixes mentioned above.
>
> I agree with Amit to start a separate thread to discuss these points.
> IMO, we can close this thread. What do you
On Wed, Jul 7, 2021 at 5:33 AM Peter Smith wrote:
> PSA my patch which includes all the fixes mentioned above.
I agree with Amit to start a separate thread to discuss these points.
IMO, we can close this thread. What do you think?
Regards,
Bharath Rupireddy.
On Wed, Jul 7, 2021 at 7:36 AM Alvaro Herrera wrote:
>
> On 2021-Jul-07, Peter Smith wrote:
>
> > 1. Zap 'opts' up-front
> >
> > + *
> > + * Caller is expected to have cleared 'opts'.
> >
> > This comment is putting the onus on the caller to "do the right thing".
> >
> > I think that hopeful
On Tue, Jul 6, 2021 at 9:24 PM Alvaro Herrera wrote:
>
> On 2021-Jul-06, Bharath Rupireddy wrote:
>
> > Thanks, Amit. I'm posting the 0002 patch which removes extra ereport
> > calls using local variables. Please review it.
>
> I looked at this the other day and I'm not sure I like it very much.
On 2021-Jul-07, Peter Smith wrote:
> 1. Zap 'opts' up-front
>
> + *
> + * Caller is expected to have cleared 'opts'.
>
> This comment is putting the onus on the caller to "do the right thing".
>
> I think that hopeful expectations about input should be removed - the
> function should just be
On Tue, Jul 6, 2021 at 6:21 PM Amit Kapila wrote:
>
> On Fri, Jul 2, 2021 at 12:36 PM Amit Kapila wrote:
> >
> > On Fri, Jul 2, 2021 at 8:35 AM Alvaro Herrera
> > wrote:
> > >
> > > > The latest patch sent by Bharath looks good to me. Would you like to
> > > > commit it or shall I take care of
On 2021-Jul-06, Bharath Rupireddy wrote:
> Thanks, Amit. I'm posting the 0002 patch which removes extra ereport
> calls using local variables. Please review it.
I looked at this the other day and I'm not sure I like it very much.
It's not making anything any simpler, it's barely saving two lines
On Tue, Jul 6, 2021 at 1:51 PM Amit Kapila wrote:
> > Okay, I'll push it early next week (by Tuesday) unless there are more
> > comments or suggestions. Thanks!
> >
>
> Pushed!
Thanks, Amit. I'm posting the 0002 patch which removes extra ereport
calls using local variables. Please review it.
On Fri, Jul 2, 2021 at 12:36 PM Amit Kapila wrote:
>
> On Fri, Jul 2, 2021 at 8:35 AM Alvaro Herrera wrote:
> >
> > > The latest patch sent by Bharath looks good to me. Would you like to
> > > commit it or shall I take care of it?
> >
> > Please, go ahead.
> >
>
> Okay, I'll push it early next
On Fri, Jul 2, 2021 at 8:35 AM Alvaro Herrera wrote:
>
> > The latest patch sent by Bharath looks good to me. Would you like to
> > commit it or shall I take care of it?
>
> Please, go ahead.
>
Okay, I'll push it early next week (by Tuesday) unless there are more
comments or suggestions. Thanks!
On 2021-Jul-02, Amit Kapila wrote:
> Yeah, that makes sense. I have removed its usage from
> CreateSubscription but I think we can get rid of it entirely as well.
Nod.
> The latest patch sent by Bharath looks good to me. Would you like to
> commit it or shall I take care of it?
Please, go
On Thu, Jul 1, 2021 at 8:00 PM Alvaro Herrera wrote:
>
> I find the business with OPT_NONE a bit uselessly verbose. It's like we
> haven't completely made up our minds that zero means no options set.
> Wouldn't it be simpler to remove that #define and leave the variable
> uninitialized until we
On Thu, Jul 1, 2021 at 6:32 PM Amit Kapila wrote:
> > 3) There's an whitespace introduced making the SUBOPT_SLOT_NAME,
> > SUBOPT_SYNCHRONOUS_COMMIT and SUBOPT_STREAMING not falling line with
> > the SUBOPT_CONNECT
> >
>
> okay, will fix it.
PSA v11 patch which also has the cleanup patch shared
I find the business with OPT_NONE a bit uselessly verbose. It's like we
haven't completely made up our minds that zero means no options set.
Wouldn't it be simpler to remove that #define and leave the variable
uninitialized until we want to set the options we want, and then use
plain assignment
On Thu, Jul 1, 2021 at 5:37 PM Bharath Rupireddy
wrote:
>
> On Thu, Jul 1, 2021 at 4:38 PM Amit Kapila wrote:
> >
> > On Wed, Jun 30, 2021 at 7:38 PM Bharath Rupireddy
> > wrote:
> > >
> > > PFA v9 patch set for further review.
> > >
> >
> > The first patch looks mostly good to me. I have made
On Thu, Jul 1, 2021 at 4:38 PM Amit Kapila wrote:
>
> On Wed, Jun 30, 2021 at 7:38 PM Bharath Rupireddy
> wrote:
> >
> > PFA v9 patch set for further review.
> >
>
> The first patch looks mostly good to me. I have made some minor
> modifications to the 0001 patch: (a) added/edited few comments,
On Wed, Jun 30, 2021 at 7:38 PM Bharath Rupireddy
wrote:
>
> PFA v9 patch set for further review.
>
The first patch looks mostly good to me. I have made some minor
modifications to the 0001 patch: (a) added/edited few comments, (b)
there is no need to initialize supported_opts variable in
On Wed, Jun 30, 2021 at 11:11 AM Amit Kapila wrote:
>
> On Tue, Jun 29, 2021 at 8:56 PM Bharath Rupireddy
> wrote:
> >
> > On Tue, Jun 29, 2021 at 4:37 PM Amit Kapila wrote:
> > > Few comments:
> > > ===
> >
> > > 2.
> > > +parse_subscription_options(List *stmt_options, SubOpts
On Wed, Jun 30, 2021 at 10:52 AM Amit Kapila wrote:
>
> On Tue, Jun 29, 2021 at 9:41 PM Alvaro Herrera
> wrote:
> >
> > On 2021-Jun-29, Bharath Rupireddy wrote:
> >
> > > On Tue, Jun 29, 2021 at 4:37 PM Amit Kapila
> > > wrote:
> > > > Few comments:
> > > > ===
> > > > 1.
> > > >
On Tue, Jun 29, 2021 at 8:56 PM Bharath Rupireddy
wrote:
>
> On Tue, Jun 29, 2021 at 4:37 PM Amit Kapila wrote:
> > Few comments:
> > ===
>
> > 2.
> > +parse_subscription_options(List *stmt_options, SubOpts *opts)
> > {
> > ListCell *lc;
> > - bool connect_given = false;
> > -
On Tue, Jun 29, 2021 at 9:41 PM Alvaro Herrera wrote:
>
> On 2021-Jun-29, Bharath Rupireddy wrote:
>
> > On Tue, Jun 29, 2021 at 4:37 PM Amit Kapila wrote:
> > > Few comments:
> > > ===
> > > 1.
> > > +typedef struct SubOpts
> > > +{
> > > + bits32 supported_opts; /* bitmap of
On 2021-Jun-29, Bharath Rupireddy wrote:
> On Tue, Jun 29, 2021 at 4:37 PM Amit Kapila wrote:
> > Few comments:
> > ===
> > 1.
> > +typedef struct SubOpts
> > +{
> > + bits32 supported_opts; /* bitmap of supported SUBOPT_* */
> > + bits32 specified_opts; /* bitmap of user specified
On Tue, Jun 29, 2021 at 4:37 PM Amit Kapila wrote:
> Few comments:
> ===
> 1.
> +typedef struct SubOpts
> +{
> + bits32 supported_opts; /* bitmap of supported SUBOPT_* */
> + bits32 specified_opts; /* bitmap of user specified SUBOPT_* */
>
> I think it will be better to not keep
On Mon, Jun 28, 2021 at 3:24 PM Bharath Rupireddy
wrote:
>
> On Fri, Jun 18, 2021 at 6:35 PM Bharath Rupireddy
> wrote:
> > > PSA v5 patch set.
> >
> > PSA v6 patch set rebased onto the latest master.
>
> PSA v7 patch set rebased onto the latest master.
>
Few comments:
===
1.
On Fri, Jun 18, 2021 at 6:35 PM Bharath Rupireddy
wrote:
> > PSA v5 patch set.
>
> PSA v6 patch set rebased onto the latest master.
PSA v7 patch set rebased onto the latest master.
With Regards,
Bharath Rupireddy.
v7-0001-Refactor-parse_subscription_options.patch
Description: Binary data
On Thu, Jun 10, 2021 at 6:36 PM Bharath Rupireddy
wrote:
>
> On Thu, Jun 10, 2021 at 9:17 AM Bharath Rupireddy
> wrote:
> > > I don't know the answer; my guess is that all this has become obsolete
> > > and the whole Assert and the dodgy comment can just be deleted.
> >
> > Hm. I get it.
At Fri, 11 Jun 2021 16:29:10 -0400, Robert Haas wrote
in
> On Tue, May 25, 2021 at 9:38 AM Alvaro Herrera
> wrote:
> > This should be okay, right? Well, almost. The problem here is if you
> > want to have a variable where you set more than one option, you have to
> > use bit-and of the enum
On Tue, May 25, 2021 at 9:38 AM Alvaro Herrera wrote:
> This should be okay, right? Well, almost. The problem here is if you
> want to have a variable where you set more than one option, you have to
> use bit-and of the enum values ... and the resulting value is no longer
> part of the enum. A
On Thu, Jun 10, 2021 at 9:17 AM Bharath Rupireddy
wrote:
> > I don't know the answer; my guess is that all this has become obsolete
> > and the whole Assert and the dodgy comment can just be deleted.
>
> Hm. I get it. Unfortunately the commit b1ff33f is missing information
> on what the coverity
On Thu, Jun 10, 2021 at 09:17:55AM +0530, Bharath Rupireddy wrote:
> Hm. I get it. Unfortunately the commit b1ff33f is missing information
> on what the coverity tool was complaining of and it has no related
> discussion at all.
This came from a FORWARD_NULL complain, due to the fact that
On Thu, Jun 10, 2021 at 8:55 AM Peter Smith wrote:
> > > 2.
> > > + /* If connect option is supported, the others also need to be. */
> > > + Assert(!IsSet(supported_opts, SUBOPT_CONNECT) ||
> > > +(IsSet(supported_opts, SUBOPT_ENABLED) &&
> > > + IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
On Thu, Jun 10, 2021 at 1:28 AM Bharath Rupireddy
wrote:
>
> On Wed, Jun 9, 2021 at 10:37 AM Peter Smith wrote:
> >
[...]
I checked the v4* patches.
Everything applies and builds and tests OK for me.
> > 2.
> > + /* If connect option is supported, the others also need to be. */
> > +
On Wed, Jun 9, 2021 at 10:37 AM Peter Smith wrote:
>
> On Wed, Jun 2, 2021 at 10:41 PM Bharath Rupireddy
> wrote:
> >
> > On Wed, Jun 2, 2021 at 11:43 AM Peter Smith wrote:
> > > Yes, it looks better, but (since the masks are all 1 bit) I was only
> > > asking why not do like:
> > >
> > > if
On Wed, Jun 2, 2021 at 10:41 PM Bharath Rupireddy
wrote:
>
> On Wed, Jun 2, 2021 at 11:43 AM Peter Smith wrote:
> > Yes, it looks better, but (since the masks are all 1 bit) I was only
> > asking why not do like:
> >
> > if (supported_opts & SUBOPT_CONNECT)
> > if (supported_opts &
On Wed, Jun 2, 2021 at 6:11 PM Bharath Rupireddy <
bharath.rupireddyforpostg...@gmail.com> wrote:
>
> On Wed, Jun 2, 2021 at 11:43 AM Peter Smith wrote:
> > Yes, it looks better, but (since the masks are all 1 bit) I was only
> > asking why not do like:
> >
> > if (supported_opts &
On Wed, Jun 2, 2021 at 11:43 AM Peter Smith wrote:
> Yes, it looks better, but (since the masks are all 1 bit) I was only
> asking why not do like:
>
> if (supported_opts & SUBOPT_CONNECT)
> if (supported_opts & SUBOPT_ENABLED)
> if (supported_opts & SUBOPT_SLOT_NAME)
> if (supported_opts &
On Wed, Jun 2, 2021 at 3:33 PM Bharath Rupireddy
wrote:
>
> > + /* If connect option is supported, the others also need to be. */
> > + Assert((supported_opts & SUBOPT_CONNECT) == 0 ||
> > +((supported_opts & SUBOPT_ENABLED) != 0 &&
> > + (supported_opts & SUBOPT_CREATE_SLOT) != 0 &&
> > +
On Wed, Jun 2, 2021 at 9:07 AM Peter Smith wrote:
>
> On Wed, Jun 2, 2021 at 12:55 AM Bharath Rupireddy
> wrote:
> >
> > On Mon, May 24, 2021 at 7:04 AM Michael Paquier wrote:
> > > Please see a3dc926 and the surrounding discussion for reasons why we've
> > > been using bitmaps for option
On Wed, Jun 2, 2021 at 12:55 AM Bharath Rupireddy
wrote:
>
> On Mon, May 24, 2021 at 7:04 AM Michael Paquier wrote:
> > Please see a3dc926 and the surrounding discussion for reasons why we've
> > been using bitmaps for option parsing lately.
>
> Thanks for the suggestion. Here's a WIP patch
On Mon, May 24, 2021 at 7:04 AM Michael Paquier wrote:
> Please see a3dc926 and the surrounding discussion for reasons why we've
> been using bitmaps for option parsing lately.
Thanks for the suggestion. Here's a WIP patch implementing the
subscription command options as bitmaps similar to
On Tue, May 25, 2021 at 7:08 PM Alvaro Herrera wrote:
> > I see that the commit a3dc926 and discussion at [1] say below respectively:
> > "All the options of those commands are changed to use hex values
> > rather than enums to reduce the risk of compatibility bugs when
> > introducing new
On 2021-May-25, Bharath Rupireddy wrote:
> On Mon, May 24, 2021 at 11:37 PM Alvaro Herrera
> wrote:
> > There's no API limitation here, since that stuff is not user-visible, so
> > it doesn't matter. If we ever need a 33rd option, we can change the
> > datatype to bits64. Any extensions
On Tue, May 25, 2021 at 11:04 AM Michael Paquier wrote:
>
> On Tue, May 25, 2021 at 10:59:37AM +0530, Bharath Rupireddy wrote:
> > I'm not able to grasp what are the incompatibilities we can have if
> > the enums are used as bit masks. It will be great if anyone throws
> > some light on this?
>
>
On Tue, May 25, 2021 at 10:59:37AM +0530, Bharath Rupireddy wrote:
> I'm not able to grasp what are the incompatibilities we can have if
> the enums are used as bit masks. It will be great if anyone throws
> some light on this?
0176753 is one example.
--
Michael
signature.asc
Description: PGP
On Mon, May 24, 2021 at 11:37 PM Alvaro Herrera wrote:
>
> On 2021-May-24, Bharath Rupireddy wrote:
>
> > On Mon, May 24, 2021 at 7:04 AM Michael Paquier wrote:
> > > What you are writing here and your comment two paragraphs above are
> > > inconsistent as you are using an enum here. Please see
On 2021-May-24, Bharath Rupireddy wrote:
> On Mon, May 24, 2021 at 7:04 AM Michael Paquier wrote:
> > What you are writing here and your comment two paragraphs above are
> > inconsistent as you are using an enum here. Please see a3dc926 and
> > the surrounding discussion for reasons why we've
On Mon, May 24, 2021 at 7:04 AM Michael Paquier wrote:
> What you are writing here and your comment two paragraphs above are
> inconsistent as you are using an enum here. Please see a3dc926 and
> the surrounding discussion for reasons why we've been using bitmaps
> for option parsing lately.
On Sat, May 22, 2021 at 01:47:24PM +0530, Bharath Rupireddy wrote:
> Thanks. I think using bitmaps would help us have clean code. This is
> also more extensible. See pseudo code at [1]. One disadvantage is that
> we might have bms_XXXfunction calls, but that's okay and it shouldn't
> add too much
On Sat, May 22, 2021 at 6:32 AM Peter Smith wrote:
> I am unsure if this will lead to better code or not; Anyway, it is
> something to consider - maybe you can experiment with it to see.
Thanks. I think using bitmaps would help us have clean code. This is
also more extensible. See pseudo code at
On Fri, May 21, 2021 at 9:21 PM Peter Smith wrote:
>
> On Thu, May 20, 2021 at 2:11 PM Bharath Rupireddy
> wrote:
> >
> > On Wed, May 19, 2021 at 6:13 PM Bharath Rupireddy
> > wrote:
> > > Thanks. I will work on the new structure ParseSubOption only for
> > > subscription options.
> >
> > PSA
On Thu, May 20, 2021 at 2:11 PM Bharath Rupireddy
wrote:
>
> On Wed, May 19, 2021 at 6:13 PM Bharath Rupireddy
> wrote:
> > Thanks. I will work on the new structure ParseSubOption only for
> > subscription options.
>
> PSA v2 patch that has changes for 1) new ParseSubOption structure 2)
> the
On Wed, May 19, 2021 at 6:13 PM Bharath Rupireddy
wrote:
> Thanks. I will work on the new structure ParseSubOption only for
> subscription options.
PSA v2 patch that has changes for 1) new ParseSubOption structure 2)
the error reporting code refactoring.
With Regards,
Bharath Rupireddy.
On Wed, May 19, 2021 at 5:56 PM Amit Kapila wrote:
>
> On Wed, May 19, 2021 at 4:42 PM Bharath Rupireddy
> wrote:
> >
> > On Wed, May 19, 2021 at 4:10 PM Amit Kapila wrote:
> > >
> > > On Wed, May 19, 2021 at 3:08 PM Bharath Rupireddy
> > > wrote:
> > > >
> > > > On Wed, May 19, 2021 at 2:33
On Wed, May 19, 2021 at 4:42 PM Bharath Rupireddy
wrote:
>
> On Wed, May 19, 2021 at 4:10 PM Amit Kapila wrote:
> >
> > On Wed, May 19, 2021 at 3:08 PM Bharath Rupireddy
> > wrote:
> > >
> > > On Wed, May 19, 2021 at 2:33 PM Amul Sul wrote:
> > > >
> > > > On Wed, May 19, 2021 at 2:09 PM
On Wed, May 19, 2021 at 4:10 PM Amit Kapila wrote:
>
> On Wed, May 19, 2021 at 3:08 PM Bharath Rupireddy
> wrote:
> >
> > On Wed, May 19, 2021 at 2:33 PM Amul Sul wrote:
> > >
> > > On Wed, May 19, 2021 at 2:09 PM Bharath Rupireddy
> > > wrote:
> > > >
> > > > Hi,
> > > >
> > > >
On Wed, May 19, 2021 at 3:08 PM Bharath Rupireddy
wrote:
>
> On Wed, May 19, 2021 at 2:33 PM Amul Sul wrote:
> >
> > On Wed, May 19, 2021 at 2:09 PM Bharath Rupireddy
> > wrote:
> > >
> > > Hi,
> > >
> > > parse_subscription_options function has some similar code when
> > > throwing errors
On Wed, May 19, 2021 at 2:33 PM Amul Sul wrote:
>
> On Wed, May 19, 2021 at 2:09 PM Bharath Rupireddy
> wrote:
> >
> > Hi,
> >
> > parse_subscription_options function has some similar code when
> > throwing errors [with the only difference in the option]. I feel we
> > could just use a variable
On Wed, May 19, 2021 at 2:09 PM Bharath Rupireddy
wrote:
>
> Hi,
>
> parse_subscription_options function has some similar code when
> throwing errors [with the only difference in the option]. I feel we
> could just use a variable for the option and use it in the error.
> While this has no benefit
Hi,
parse_subscription_options function has some similar code when
throwing errors [with the only difference in the option]. I feel we
could just use a variable for the option and use it in the error.
While this has no benefit at all, it saves some LOC and makes the code
look better with lesser
60 matches
Mail list logo
