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.koval@postgrespro.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/