Re: TRUNCATE on foreign table

On Sun, Apr 11, 2021 at 9:47 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> Find attached language fixes.

Thanks for the patches.

> I'm also proposing to convert an if/else to an switch(), since I don't like
> "if/else if" without an "else", and since the compiler can warn about missing
> enum values in switch cases.

I think we have a good bunch of if, else-if (without else) in the code
base, and I'm not sure the compilers have warned about them. Apart
from that whether if-else or switch-case is just a coding choice. And
we have only two values for DropBehavior enum i.e DROP_RESTRICT and
DROP_CASCADE(I'm not sure we will extend this enum to have more
values), if we have more then switch case would have looked cleaner.
But IMO, existing if and else-if would be good. Having said that, it's
up to the committer which one they think better in this case.

> You could also write:
> | Assert(behavior == DROP_RESTRICT || behavior == DROP_CASCADE)

IMO, we don't need to assert on behaviour as we just carry that
variable from ExecuteTruncateGuts to postgresExecForeignTruncate
without any modifications. And ExecuteTruncateGuts would get it from
the syntaxer, so no point it will have a different value than
DROP_RESTRICT or DROP_CASCADE.

> Also, you currently test:
> > +             if (extra & TRUNCATE_REL_CONTEXT_ONLY)
>
> but TRUNCATE_REL_ aren't indepedent bits, so shouldn't be tested with "&".

Yeah this is an issue. We could just change the #defines to values
0x0001, 0x0002, 0x0004, 0x0008 ... 0x0020 and so on and then testing
with & would work. So, this way, more than option can be multiplexed
into the same int value. To multiplex, we need to think: will there be
a scenario where a single rel in the truncate can have multiple
options at a time and do we want to distinguish these options while
deparsing?

#define TRUNCATE_REL_CONTEXT_NORMAL      0x0001 /* specified without
ONLY clause */
#define TRUNCATE_REL_CONTEXT_ONLY         0x0002 /* specified with
ONLY clause */
#define TRUNCATE_REL_CONTEXT_CASCADING    0x0004   /* not specified
but truncated

And I'm not sure what's the agreement on retaining or removing #define
values, currently I see only TRUNCATE_REL_CONTEXT_ONLY is being used,
others are just being set but not used. As I said upthread, it will be
good to remove the unused macros/enums, retain only the ones that are
used, especially TRUNCATE_REL_CONTEXT_CASCADING this is not required I
feel, because we can add the child partitions that are foreign tables
to relids as just normal foreign tables with TRUNCATE_REL_CONTEXT_ONLY
option.

On the patches:
0001-WIP-doc-review-Allow-TRUNCATE-command-to-truncate-fo.patch ---> LGTM.
0002-Convert-an-if-else-if-without-an-else-to-a-switch.patch. --> IMO,
we can ignore this patch.
0003-Test-integer-using-and-not.patch --> if we redefine the marcos to
multiplex them into a single int value, we don't need this patch.

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