Обсуждение: Remove redundant variable from transformCreateStmt

Hi,

Attached patch removes "is_foreign_table" from transformCreateStmt()
since it already has cxt.isforeign that can serve the same purpose.

Regards,
Amul

On Thu, Apr 15, 2021 at 5:04 PM Amul Sul <sulamul@gmail.com> wrote:
>
> Hi,
>
> Attached patch removes "is_foreign_table" from transformCreateStmt()
> since it already has cxt.isforeign that can serve the same purpose.

Yeah having that variable as "is_foreign_table" doesn't make sense
when we have the info in ctx. I'm wondering whether we can do the
following (like transformFKConstraints). It will be more readable and
we could also add more comments on why we don't skip validation for
check constraints i.e. constraint->skip_validation = false in case for
foreign tables.

bool skip_validation = true;
    if (IsA(stmt, CreateForeignTableStmt))
    {
        cxt.stmtType = "CREATE FOREIGN TABLE";
        cxt.isforeign = true;
        skip_validation = false;    ----> <<<add comments here>>>
    }
transformCheckConstraints(&cxt, skip_validation);

Alternatively, we could also remove skipValidation function parameter
(since transformCheckConstraints is a static function, I think it's
okay) and modify transformCheckConstraints, then we can do following:

In transformCreateStmt:
if (!ctx.isforeign)
  transformCheckConstraints(&ctx);

In transformAlterTableStmt: we can remove transformCheckConstraints
entirely because calling transformCheckConstraints with skipValidation
= false does nothing and has no value. This way we could save a
function call.

I prefer removing the skipValidation parameter from
transformCheckConstraints. Others might have different opinions.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Apr 15, 2021 at 5:47 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Thu, Apr 15, 2021 at 5:04 PM Amul Sul <sulamul@gmail.com> wrote:
> >
> > Hi,
> >
> > Attached patch removes "is_foreign_table" from transformCreateStmt()
> > since it already has cxt.isforeign that can serve the same purpose.
>
> Yeah having that variable as "is_foreign_table" doesn't make sense
> when we have the info in ctx. I'm wondering whether we can do the
> following (like transformFKConstraints). It will be more readable and
> we could also add more comments on why we don't skip validation for
> check constraints i.e. constraint->skip_validation = false in case for
> foreign tables.
>
> bool skip_validation = true;
>     if (IsA(stmt, CreateForeignTableStmt))
>     {
>         cxt.stmtType = "CREATE FOREIGN TABLE";
>         cxt.isforeign = true;
>         skip_validation = false;    ----> <<<add comments here>>>
>     }
> transformCheckConstraints(&cxt, skip_validation);
>
> Alternatively, we could also remove skipValidation function parameter
> (since transformCheckConstraints is a static function, I think it's
> okay) and modify transformCheckConstraints, then we can do following:
>
> In transformCreateStmt:
> if (!ctx.isforeign)
>   transformCheckConstraints(&ctx);
>
> In transformAlterTableStmt: we can remove transformCheckConstraints
> entirely because calling transformCheckConstraints with skipValidation
> = false does nothing and has no value. This way we could save a
> function call.
>
> I prefer removing the skipValidation parameter from
> transformCheckConstraints. Others might have different opinions.
>

Then we also need to remove the transformCheckConstraints() dummy call
from transformAlterTableStmt() which was added for the readability.
Also, this change to transformCheckConstraints() will make it
inconsistent with transformFKConstraints().

I think we shouldn't worry too much about this function call overhead
here since this is a slow utility path, and that is the reason the
current structure doesn't really bother me.

Regards,
Amul

On Thu, Apr 15, 2021 at 8:40 PM Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:
> IMHO, I think the idea here was to just get rid of an unnecessary variable
> rather than refactoring.
>
> On Thu, Apr 15, 2021 at 5:48 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
>>
>> On Thu, Apr 15, 2021 at 5:04 PM Amul Sul <sulamul@gmail.com> wrote:
>> >
>> > Hi,
>> >
>> > Attached patch removes "is_foreign_table" from transformCreateStmt()
>> > since it already has cxt.isforeign that can serve the same purpose.
>>
>> Yeah having that variable as "is_foreign_table" doesn't make sense
>> when we have the info in ctx. I'm wondering whether we can do the
>> following (like transformFKConstraints). It will be more readable and
>> we could also add more comments on why we don't skip validation for
>> check constraints i.e. constraint->skip_validation = false in case for
>> foreign tables.
>
> To address your concern here, I think it can be addressed by adding a comment
> just before we make a call to transformCheckConstraints().

+1. The comment  * If creating a new table (but not a foreign table),
we can safely skip * in transformCheckConstraints just says that we
don't mark skip_validation = true for foreign tables. But the
discussion that led to the commit 86705aa8 [1] has the information as
to why it is so. Although, I have not gone through it entirely, having
something like "newly created foreign tables can have data at the
moment they created, so the constraint validation cannot be skipped"
in transformCreateStmt before calling transformCheckConstraints gives
an idea as to why we don't skip validation.

[1] - https://www.postgresql.org/message-id/flat/d2b7419f-4a71-cf86-cc99-bfd0f359a1ea%40lab.ntt.co.jp

> I think this is intentional, to keep the code consistent with the CREATE
> TABLE path i.e. transformCreateStmt(). Here is what the comment atop
> transformCheckConstraints() reads:
>
> /*
>  * transformCheckConstraints
>  * handle CHECK constraints
>  *
>  * Right now, there's nothing to do here when called from ALTER TABLE,
>  * but the other constraint-transformation functions are called in both
>  * the CREATE TABLE and ALTER TABLE paths, so do the same here, and just
>  * don't do anything if we're not authorized to skip validation.
>  */

Yeah, I re-read it and it looks like it's intentional for consistency reasons.

I'm not opposed to this patch as it clearly removes an unnecessary variable.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Apr 16, 2021 at 6:26 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Thu, Apr 15, 2021 at 8:40 PM Jeevan Ladhe
> <jeevan.ladhe@enterprisedb.com> wrote:
> > IMHO, I think the idea here was to just get rid of an unnecessary variable
> > rather than refactoring.
> >
> > On Thu, Apr 15, 2021 at 5:48 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> >>
> >> On Thu, Apr 15, 2021 at 5:04 PM Amul Sul <sulamul@gmail.com> wrote:
> >> >
> >> > Hi,
> >> >
> >> > Attached patch removes "is_foreign_table" from transformCreateStmt()
> >> > since it already has cxt.isforeign that can serve the same purpose.
> >>
> >> Yeah having that variable as "is_foreign_table" doesn't make sense
> >> when we have the info in ctx. I'm wondering whether we can do the
> >> following (like transformFKConstraints). It will be more readable and
> >> we could also add more comments on why we don't skip validation for
> >> check constraints i.e. constraint->skip_validation = false in case for
> >> foreign tables.
> >
> > To address your concern here, I think it can be addressed by adding a comment
> > just before we make a call to transformCheckConstraints().
>
> +1.

Ok, added the comment in the attached version.

Thanks Jeevan & Bharat for the review.

Regards,
Amul

On Mon, Apr 19, 2021 at 9:28 AM Amul Sul <sulamul@gmail.com> wrote:
>
> On Fri, Apr 16, 2021 at 6:26 AM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Thu, Apr 15, 2021 at 8:40 PM Jeevan Ladhe
> > <jeevan.ladhe@enterprisedb.com> wrote:
> > > IMHO, I think the idea here was to just get rid of an unnecessary variable
> > > rather than refactoring.
> > >
> > > On Thu, Apr 15, 2021 at 5:48 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> > >>
> > >> On Thu, Apr 15, 2021 at 5:04 PM Amul Sul <sulamul@gmail.com> wrote:
> > >> >
> > >> > Hi,
> > >> >
> > >> > Attached patch removes "is_foreign_table" from transformCreateStmt()
> > >> > since it already has cxt.isforeign that can serve the same purpose.
> > >>
> > >> Yeah having that variable as "is_foreign_table" doesn't make sense
> > >> when we have the info in ctx. I'm wondering whether we can do the
> > >> following (like transformFKConstraints). It will be more readable and
> > >> we could also add more comments on why we don't skip validation for
> > >> check constraints i.e. constraint->skip_validation = false in case for
> > >> foreign tables.
> > >
> > > To address your concern here, I think it can be addressed by adding a comment
> > > just before we make a call to transformCheckConstraints().
> >
> > +1.
>
> Ok, added the comment in the attached version.

Kindly ignore the previous version -- has unnecessary change.
See the attached.

Regards,
Amul

On Mon, Apr 19, 2021 at 9:32 AM Amul Sul <sulamul@gmail.com> wrote:
> Kindly ignore the previous version -- has unnecessary change.
> See the attached.

Thanks for the patch!

How about a slight rewording of the added comment to "Constraints
validation can be skipped for a newly created table as it contains no
data. However, this is not necessarily true for a foreign table."?

You may want to add it to the commitfest if not done already. And I
don't think we need to backpatch this as it's not critical.


With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Apr 19, 2021 at 11:05 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Mon, Apr 19, 2021 at 9:32 AM Amul Sul <sulamul@gmail.com> wrote:
> > Kindly ignore the previous version -- has unnecessary change.
> > See the attached.
>
> Thanks for the patch!
>
> How about a slight rewording of the added comment to "Constraints
> validation can be skipped for a newly created table as it contains no
> data. However, this is not necessarily true for a foreign table."?
>

Well, wording is quite subjective, let's leave this to the committer
for the final decision, I don't see anything wrong with it.

> You may want to add it to the commitfest if not done already. And I
> don't think we need to backpatch this as it's not critical.

This is not fixing anything so not a relevant candidate for the backporting.

Regards,
Amul

I'd do it like this.  Note I removed an if/else block in addition to
your changes.

I couldn't convince myself that this is worth pushing though; either we
push it to all branches (which seems unwarranted) or we create
back-patching hazards.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I'd do it like this.  Note I removed an if/else block in addition to
> your changes.

> I couldn't convince myself that this is worth pushing though; either we
> push it to all branches (which seems unwarranted) or we create
> back-patching hazards.

Yeah ... an advantage of the if/else coding is that it'd likely be
simple to extend to cover additional statement types, should we ever
wish to do that.  The rendering you have here is nice and compact,
but it would not scale up well.

            regards, tom lane

On 2021-Apr-29, Tom Lane wrote:

> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > I'd do it like this.  Note I removed an if/else block in addition to
> > your changes.
> 
> > I couldn't convince myself that this is worth pushing though; either we
> > push it to all branches (which seems unwarranted) or we create
> > back-patching hazards.
> 
> Yeah ... an advantage of the if/else coding is that it'd likely be
> simple to extend to cover additional statement types, should we ever
> wish to do that.  The rendering you have here is nice and compact,
> but it would not scale up well.

That makes sense.  But that part is not in Amul's patch -- he was only
on about removing the is_foreign_table Boolean.  If I remove the if/else
block change, does the rest of the patch looks something we'd want to
have?  I kinda agree that the redundant variable is "ugly".  Is it worth
removing?  My hunch is no.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W

On Thu, Apr 29, 2021 at 02:39:42PM -0400, Alvaro Herrera wrote:
> On 2021-Apr-29, Tom Lane wrote:
> > Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > > I'd do it like this.  Note I removed an if/else block in addition to
> > > your changes.
> > 
> > > I couldn't convince myself that this is worth pushing though; either we
> > > push it to all branches (which seems unwarranted) or we create
> > > back-patching hazards.
> > 
> > Yeah ... an advantage of the if/else coding is that it'd likely be
> > simple to extend to cover additional statement types, should we ever
> > wish to do that.  The rendering you have here is nice and compact,
> > but it would not scale up well.
> 
> That makes sense.  But that part is not in Amul's patch -- he was only
> on about removing the is_foreign_table Boolean.  If I remove the if/else
> block change, does the rest of the patch looks something we'd want to
> have?  I kinda agree that the redundant variable is "ugly".  Is it worth
> removing?  My hunch is no.

Getting rid of a redundant, boolean variable is good not because it's more
efficient but because it's one fewer LOC to read and maintain (and an
opportunity for inconsistency, I suppose).

Also, this is a roundabout and too-verbose way to invert a boolean:
| transformCheckConstraints(&cxt, !is_foreign_table ? true : false);

-- 
Justin

PS. It's also not pythonic ;)

On Fri, Apr 30, 2021 at 7:07 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Thu, Apr 29, 2021 at 02:39:42PM -0400, Alvaro Herrera wrote:
> > On 2021-Apr-29, Tom Lane wrote:
> > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > > > I'd do it like this.  Note I removed an if/else block in addition to
> > > > your changes.
> > >
> > > > I couldn't convince myself that this is worth pushing though; either we
> > > > push it to all branches (which seems unwarranted) or we create
> > > > back-patching hazards.
> > >
> > > Yeah ... an advantage of the if/else coding is that it'd likely be
> > > simple to extend to cover additional statement types, should we ever
> > > wish to do that.  The rendering you have here is nice and compact,
> > > but it would not scale up well.
> >
> > That makes sense.  But that part is not in Amul's patch -- he was only
> > on about removing the is_foreign_table Boolean.  If I remove the if/else
> > block change, does the rest of the patch looks something we'd want to
> > have?  I kinda agree that the redundant variable is "ugly".  Is it worth
> > removing?  My hunch is no.
>
> Getting rid of a redundant, boolean variable is good not because it's more
> efficient but because it's one fewer LOC to read and maintain (and an
> opportunity for inconsistency, I suppose).

Yes.

> Also, this is a roundabout and too-verbose way to invert a boolean:
> | transformCheckConstraints(&cxt, !is_foreign_table ? true : false);

I agree to remove only the redundant variable, is_foreign_table but
not the if else block as Tom said: it's not scalable. We don't need to
back patch this change.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Apr 30, 2021 at 10:49 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Fri, Apr 30, 2021 at 7:07 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> >
> > On Thu, Apr 29, 2021 at 02:39:42PM -0400, Alvaro Herrera wrote:
> > > On 2021-Apr-29, Tom Lane wrote:
> > > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > > > > I'd do it like this.  Note I removed an if/else block in addition to
> > > > > your changes.
> > > >
> > > > > I couldn't convince myself that this is worth pushing though; either we
> > > > > push it to all branches (which seems unwarranted) or we create
> > > > > back-patching hazards.
> > > >
> > > > Yeah ... an advantage of the if/else coding is that it'd likely be
> > > > simple to extend to cover additional statement types, should we ever
> > > > wish to do that.  The rendering you have here is nice and compact,
> > > > but it would not scale up well.
> > >
> > > That makes sense.  But that part is not in Amul's patch -- he was only
> > > on about removing the is_foreign_table Boolean.  If I remove the if/else
> > > block change, does the rest of the patch looks something we'd want to
> > > have?  I kinda agree that the redundant variable is "ugly".  Is it worth
> > > removing?  My hunch is no.
> >
> > Getting rid of a redundant, boolean variable is good not because it's more
> > efficient but because it's one fewer LOC to read and maintain (and an
> > opportunity for inconsistency, I suppose).
>
> Yes.
>
> > Also, this is a roundabout and too-verbose way to invert a boolean:
> > | transformCheckConstraints(&cxt, !is_foreign_table ? true : false);
>
> I agree to remove only the redundant variable, is_foreign_table but
> not the if else block as Tom said: it's not scalable.

+1.

Regards,
Amul

On 2021-Apr-29, Justin Pryzby wrote:

> Getting rid of a redundant, boolean variable is good not because it's more
> efficient but because it's one fewer LOC to read and maintain (and an
> opportunity for inconsistency, I suppose).

Makes sense. Pushed.  Thanks everyone.

> Also, this is a roundabout and too-verbose way to invert a boolean:
> | transformCheckConstraints(&cxt, !is_foreign_table ? true : false);

It is, yeah.

> PS. It's also not pythonic ;)

Umm.  If you say so.  But this is not Python ...

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"El hombre nunca sabe de lo que es capaz hasta que lo intenta" (C. Dickens)