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().
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
