Hi
Another thing that should be described there is that this falls
> outside of the transaction flow, i.e. it's changes are not reverted on
> ROLLBACK. But that leaves an important consideration: What happens
> when an error occurs on the server during handling of this message
> (e.g. the GUC does not exist or an OOM is triggered). Is any open
> transaction aborted in that case? If not, we should have a test for
> that.
>
I tested this scenario. I had to modify message handling to fix warning
"message type 0x5a arrived from server while idle"
But if this is inside a transaction, the transaction is aborted.
>
> + if (PQresultStatus(res) != PGRES_COMMAND_OK)
> + pg_fatal("failed to link custom variable: %s",
> PQerrorMessage(conn));
> + PQclear(res);
>
done
>
> The tests should also change the config after running
> PQlinkParameterStatus/PQunlinkParameterStatus to show that the guc is
> reported then or not reported then.
>
done
>
> + if (!PQsendTypedCommand(conn, PqMsg_ReportGUC, 't', paramName))
> + return NULL;
>
>
> I think we'll need some bikeshedding on what the protocol message
> should look like exactly. I'm not entirely sure what is the most
> sensible here, so please treat everything I write next as
> suggestions/discussion:
> I see that you're piggy-backing on PQsendTypedCommand, which seems
> nice to avoid code duplication. It has one downside though: not every
> type, is valid for each command anymore.
> One way to avoid that would be to not introduce a new command, but
> only add a new type that is understood by Describe and Close, e.g. a
> 'G' (for GUC). Then PqMsg_Describe, G would become the equivalent of
> what'the current patch its PqMsg_ReportGUC, 't' and PqMsg_Close, G
> would be the same as PqMsg_ReportGUC, 'f'.
>
I am sorry, I don't understand this idea?
>
> The rest of this email assumes that we're keeping your current
> proposal for the protocol message, so it might not make sense to
> address all of this feedback, in case we're still going to change the
> protocol:
>
> + if (is_set == 't')
> + {
> + SetGUCOptionFlag(name, GUC_REPORT);
> + status = "SET REPORT_GUC";
> + }
> + else
> + {
> + UnsetGUCOptionFlag(name, GUC_REPORT);
> + status = "UNSET REPORT_GUC";
> + }
>
> I think we should be strict about what we accept here. Any other value
> than 'f'/'t' for is_set should result in an error imho.
>
done
