On 2021/04/11 19:15, Bharath Rupireddy wrote:
On Sun, Apr 11, 2021 at 9:47 AM Justin Pryzby <pry...@telsasoft.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
