Re: Remove redundant variable from transformCreateStmt
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(, !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 Herrera39°49'30"S 73°17'W "El hombre nunca sabe de lo que es capaz hasta que lo intenta" (C. Dickens)
Re: Remove redundant variable from transformCreateStmt
On Fri, Apr 30, 2021 at 10:49 AM Bharath Rupireddy wrote: > > On Fri, Apr 30, 2021 at 7:07 AM Justin Pryzby wrote: > > > > On Thu, Apr 29, 2021 at 02:39:42PM -0400, Alvaro Herrera wrote: > > > On 2021-Apr-29, Tom Lane wrote: > > > > Alvaro Herrera 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(, !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
Re: Remove redundant variable from transformCreateStmt
On Fri, Apr 30, 2021 at 7:07 AM Justin Pryzby wrote: > > On Thu, Apr 29, 2021 at 02:39:42PM -0400, Alvaro Herrera wrote: > > On 2021-Apr-29, Tom Lane wrote: > > > Alvaro Herrera 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(, !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
Re: Remove redundant variable from transformCreateStmt
On Thu, Apr 29, 2021 at 02:39:42PM -0400, Alvaro Herrera wrote: > On 2021-Apr-29, Tom Lane wrote: > > Alvaro Herrera 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(, !is_foreign_table ? true : false); -- Justin PS. It's also not pythonic ;)
Re: Remove redundant variable from transformCreateStmt
On 2021-Apr-29, Tom Lane wrote: > Alvaro Herrera 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 Herrera39°49'30"S 73°17'W
Re: Remove redundant variable from transformCreateStmt
Alvaro Herrera 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
Re: Remove redundant variable from transformCreateStmt
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 Herrera39°49'30"S 73°17'W
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 9dd30370da..2f20d81470 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -176,7 +176,6 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
Oid namespaceid;
Oid existing_relid;
ParseCallbackState pcbstate;
- bool is_foreign_table = IsA(stmt, CreateForeignTableStmt);
/*
* We must not scribble on the passed-in CreateStmt, so copy it. (This is
@@ -227,16 +226,8 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
/* Set up CreateStmtContext */
cxt.pstate = pstate;
- if (IsA(stmt, CreateForeignTableStmt))
- {
- cxt.stmtType = "CREATE FOREIGN TABLE";
- cxt.isforeign = true;
- }
- else
- {
- cxt.stmtType = "CREATE TABLE";
- cxt.isforeign = false;
- }
+ cxt.isforeign = IsA(stmt, CreateForeignTableStmt);
+ cxt.stmtType = cxt.isforeign ? "CREATE FOREIGN TABLE" : "CREATE TABLE";
cxt.relation = stmt->relation;
cxt.rel = NULL;
cxt.inhRelations = stmt->inhRelations;
@@ -333,8 +324,11 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
/*
* Postprocess check constraints.
+ *
+ * For regular tables all constraints can be marked valid immediately,
+ * because the table must be empty. Not so for foreign tables.
*/
- transformCheckConstraints(, !is_foreign_table ? true : false);
+ transformCheckConstraints(, !cxt.isforeign);
/*
* Postprocess extended statistics.
Re: Remove redundant variable from transformCreateStmt
On Mon, Apr 19, 2021 at 11:05 AM Bharath Rupireddy wrote: > > On Mon, Apr 19, 2021 at 9:32 AM Amul Sul 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
Re: Remove redundant variable from transformCreateStmt
On Mon, Apr 19, 2021 at 9:32 AM Amul Sul 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
Re: Remove redundant variable from transformCreateStmt
On Mon, Apr 19, 2021 at 9:28 AM Amul Sul wrote: > > On Fri, Apr 16, 2021 at 6:26 AM Bharath Rupireddy > wrote: > > > > On Thu, Apr 15, 2021 at 8:40 PM Jeevan Ladhe > > 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 > > > wrote: > > >> > > >> On Thu, Apr 15, 2021 at 5:04 PM Amul Sul 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 diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c index 9dd30370dae..9aaa4bde278 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -176,7 +176,6 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString) Oid namespaceid; Oid existing_relid; ParseCallbackState pcbstate; - bool is_foreign_table = IsA(stmt, CreateForeignTableStmt); /* * We must not scribble on the passed-in CreateStmt, so copy it. (This is @@ -333,8 +332,12 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString) /* * Postprocess check constraints. + * + * For a table, the constraint can be considered validated immediately, + * because the table must be empty. But for a foreign table this is not + * necessarily the case. */ - transformCheckConstraints(, !is_foreign_table ? true : false); + transformCheckConstraints(, !cxt.isforeign); /* * Postprocess extended statistics.
Re: Remove redundant variable from transformCreateStmt
On Fri, Apr 16, 2021 at 6:26 AM Bharath Rupireddy wrote: > > On Thu, Apr 15, 2021 at 8:40 PM Jeevan Ladhe > 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 > > wrote: > >> > >> On Thu, Apr 15, 2021 at 5:04 PM Amul Sul 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 diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c index 9dd30370dae..17182500c61 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -176,7 +176,6 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString) Oid namespaceid; Oid existing_relid; ParseCallbackState pcbstate; - bool is_foreign_table = IsA(stmt, CreateForeignTableStmt); /* * We must not scribble on the passed-in CreateStmt, so copy it. (This is @@ -327,14 +326,18 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString) cxt.alist = list_concat(cxt.alist, cxt.likeclauses); /* - * Postprocess foreign-key constraints. + * Postprocess foreign-key and check constraints. */ transformFKConstraints(, true, false); /* * Postprocess check constraints. + * + * For a table, the constraint can be considered validated immediately, + * because the table must be empty. But for a foreign table this is not + * necessarily the case. */ - transformCheckConstraints(, !is_foreign_table ? true : false); + transformCheckConstraints(, !cxt.isforeign); /* * Postprocess extended statistics.
Re: Remove redundant variable from transformCreateStmt
On Thu, Apr 15, 2021 at 8:40 PM Jeevan Ladhe 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 > wrote: >> >> On Thu, Apr 15, 2021 at 5:04 PM Amul Sul 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
Re: Remove redundant variable from transformCreateStmt
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.rupireddyforpostg...@gmail.com> wrote: > On Thu, Apr 15, 2021 at 5:04 PM Amul Sul 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(). 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. > 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. */ This was originally discussed in thread[1] and commit: f27a6b15e6566fba7748d0d9a3fc5bcfd52c4a1b [1] https://www.postgresql.org/message-id/flat/1238779931.11913728.1449143089410.JavaMail.yahoo%40mail.yahoo.com#f2d8318b6beef37dfff06baa9a1538b7 Regards, Jeevan Ladhe
Re: Remove redundant variable from transformCreateStmt
On Thu, Apr 15, 2021 at 5:47 PM Bharath Rupireddy
wrote:
>
> On Thu, Apr 15, 2021 at 5:04 PM Amul Sul 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;> <<>>
> }
> transformCheckConstraints(, 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();
>
> 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
Re: Remove redundant variable from transformCreateStmt
On Thu, Apr 15, 2021 at 5:04 PM Amul Sul 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;> <<>>
}
transformCheckConstraints(, 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();
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
Re: Remove redundant variable from transformCreateStmt
Thanks Amul, this looks pretty straight forward. LGTM. I have also run the regression on master and seems good. Regards, Jeevan Ladhe
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 diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c index 9dd30370dae..97e6d65158c 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -176,7 +176,6 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString) Oid namespaceid; Oid existing_relid; ParseCallbackState pcbstate; - bool is_foreign_table = IsA(stmt, CreateForeignTableStmt); /* * We must not scribble on the passed-in CreateStmt, so copy it. (This is @@ -334,7 +333,7 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString) /* * Postprocess check constraints. */ - transformCheckConstraints(, !is_foreign_table ? true : false); + transformCheckConstraints(, !cxt.isforeign); /* * Postprocess extended statistics.
