Re: TRUNCATE on foreign table

On 2021/04/11 19:15, Bharath Rupireddy wrote:
> On Sun, Apr 11, 2021 at 9:47 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>> Find attached language fixes.
>
> Thanks for the patches.

Thanks for the patches!

0001 patch basically looks good to me.

+ <literal>behavior</literal> must be specified as
+ <literal>DROP_RESTRICT</literal> or <literal>DROP_CASCADE</literal>.
+ If specified as <literal>DROP_RESTRICT</literal>, the
+ <literal>RESTRICT</literal> option will be included in the
<command>TRUNCATE</command> command.
+ If specified as <literal>DROP_CASCADE</literal>, the
+ <literal>CASCADE</literal> option will be included.

Maybe "will be included" is confusing? Because FDW might not include
the RESTRICT in the TRUNCATE command that it will issue
when DROP_RESTRICT is specified, for example. To be more precise,
we should document something like "If specified as DROP_RESTRICT,
the RESTRICT option was included in the original TRUNCATE command"?

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

Either works at least for me. Also for now I have no strong opinion
to change the condition so that it uses switch().

>> 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 "&".

You're right.

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

Our current consensus seems that TRUNCATE_REL_CONTEXT_NORMAL and
TRUNCATE_REL_CONTEXT_CASCADING should be removed because they are not used.
Since Kaigai-san thinks to remove the extra information at all,
I guess he also agrees to remove those both TRUNCATE_REL_CONTEXT_NORMAL
and _CASCADING. If this is right, we should apply 0003 patch and remove
those two macro values. Or we should make the extra info boolean value
instead of int, i.e., it indicates whether ONLY was specified or not.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION