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
