Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

On Mon, Feb 7, 2022 at 6:21 AM Dmitry Koval <d(dot)koval(at)postgrespro(dot)ru> wrote:
>
> Hi,
>
> I a bit changed Masahiko Sawada's patch with using Dilip Kumar's
> recommendations (see it in attachment).

Thank you for updating the patch!

+
+ if (source != PGC_S_DEFAULT)
+ {
+ /* Release newextra as we use reset_extra */
+ if (newextra)
+ free(newextra);
+
+ newextra = conf->reset_extra;
+ source = conf->gen.reset_source;
+ context = conf->gen.reset_scontext;
+ }

I think it's better to check if "!extra_field_used(&conf->gen,
newextra)" before freeing newextra because otherwise, it's possible
that we free reset_extra. I've updated an updated patch, please check
it.

>
> About testing.
> The most simple path to testing of command "RESET <GUC_string_variable>"
> (with error) that I found:
> ----
> 1) need to modify "postgresql.conf". Add string:
>
> default_table_access_method = 'heapXXX'
>
> 2) start PostgreSQL;
>
> 3) start "psql" and execute command:
>
> RESET default_table_access_method;
>
> After that we get an error:
>
> ERROR: invalid value for parameter "default_table_access_method": "heapXXX"
> DETAIL: Table access method "heapXXX" does not exist.
>
> Without attached patch command "RESET default_table_access_method;"
> returns no error.
> ----
> Probably no reason to create new tap-test for this case...

Yes, I think we need regression tests at least for
transaction_isolation since it leads to an assertion failure. And this
new test covers the change that the patch made.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/