On Mon, Jun 28, 2021 at 3:24 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Fri, Jun 18, 2021 at 6:35 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > PSA v5 patch set.
> >
> > PSA v6 patch set rebased onto the latest master.
>
> PSA v7 patch set rebased onto the latest master.
>
Few comments:
===============
1.
+typedef struct SubOpts
+{
+ bits32 supported_opts; /* bitmap of supported SUBOPT_* */
+ bits32 specified_opts; /* bitmap of user specified SUBOPT_* */
I think it will be better to not keep these as part of this structure.
Is there a reason for doing so?
2.
+parse_subscription_options(List *stmt_options, SubOpts *opts)
{
ListCell *lc;
- bool connect_given = false;
- bool create_slot_given = false;
- bool copy_data_given = false;
- bool refresh_given = false;
+ bits32 supported_opts;
+ bits32 specified_opts;
- /* If connect is specified, the others also need to be. */
- Assert(!connect || (enabled && create_slot && copy_data));
I am not sure whether removing this assertion will bring back the
coverity error for which it was added but I see that the reason for
which it was added still holds true. The same is explained by Michael
as well in his email [1]. I think it is better to keep an equivalent
assert.
3.
* Since not all options can be specified in both commands, this function
* will report an error on options if the target output pointer is NULL to
* accommodate that.
*/
static void
parse_subscription_options(List *stmt_options, SubOpts *opts)
The comment above this function doesn't seem to match with the new
code. I think here it is referring to the mutually exclusive errors in
the function. If you agree with that, can we change the comment to
something like: "Since not all options can be specified in both
commands, this function will report an error if mutually exclusive
options are specified."
[1] - https://www.postgresql.org/message-id/YMGSbdV1tMTJroA6%40paquier.xyz
--
With Regards,
Amit Kapila.