Re: Remove redundant variable from transformCreateStmt

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