Custom GUCs still a bit broken

Started by Andrew Dunstanalmost 16 years ago6 messages
#1Andrew Dunstan
Andrew Dunstan
andrew@dunslane.net

It seems like Custom GUCs are still in need of some work, as shown in my
recent email. In particular, they are not transaction safe - if a
transaction attempts to do DefineCustomFooVariable() and that
transaction aborts, the placeholder setting that it used is already gone
by the time it tries to roll back GUC settings. I think this code at the
end of define_custom_variable()

/*
* Free up as much as we conveniently can of the placeholder
structure
* (this neglects any stack items...)
*/
set_string_field(pHolder, pHolder->variable, NULL);
set_string_field(pHolder, &pHolder->reset_val, NULL);

free(pHolder);

needs to be removed and instead we need to save pHolder in a list along
with the GUC level, to be processed later by AtEOXact_GUC(), which would
do the right thing according to whether or not it had a commit or an abort.

I want to get this fixed before we consider custom settings for plperl
that have possible security implications.

Thoughts?

cheers

andrew

#2Bruce Momjian
Bruce Momjian
bruce@momjian.us
In reply to: Andrew Dunstan (#1)
Re: Custom GUCs still a bit broken

Where are we on this?

---------------------------------------------------------------------------

Andrew Dunstan wrote:

It seems like Custom GUCs are still in need of some work, as shown in my
recent email. In particular, they are not transaction safe - if a
transaction attempts to do DefineCustomFooVariable() and that
transaction aborts, the placeholder setting that it used is already gone
by the time it tries to roll back GUC settings. I think this code at the
end of define_custom_variable()

/*
* Free up as much as we conveniently can of the placeholder
structure
* (this neglects any stack items...)
*/
set_string_field(pHolder, pHolder->variable, NULL);
set_string_field(pHolder, &pHolder->reset_val, NULL);

free(pHolder);

needs to be removed and instead we need to save pHolder in a list along
with the GUC level, to be processed later by AtEOXact_GUC(), which would
do the right thing according to whether or not it had a commit or an abort.

I want to get this fixed before we consider custom settings for plperl
that have possible security implications.

Thoughts?

cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do

#3Andrew Dunstan
Andrew Dunstan
andrew@dunslane.net
In reply to: Bruce Momjian (#2)
Re: Custom GUCs still a bit broken

Nowhere, really. I tried to fix it, but could not come up with anything
remotely clean.

cheers

andrew

Bruce Momjian wrote:

Show quoted text

Where are we on this?

---------------------------------------------------------------------------

Andrew Dunstan wrote:

It seems like Custom GUCs are still in need of some work, as shown in my
recent email. In particular, they are not transaction safe - if a
transaction attempts to do DefineCustomFooVariable() and that
transaction aborts, the placeholder setting that it used is already gone
by the time it tries to roll back GUC settings. I think this code at the
end of define_custom_variable()

/*
* Free up as much as we conveniently can of the placeholder
structure
* (this neglects any stack items...)
*/
set_string_field(pHolder, pHolder->variable, NULL);
set_string_field(pHolder, &pHolder->reset_val, NULL);

free(pHolder);

needs to be removed and instead we need to save pHolder in a list along
with the GUC level, to be processed later by AtEOXact_GUC(), which would
do the right thing according to whether or not it had a commit or an abort.

I want to get this fixed before we consider custom settings for plperl
that have possible security implications.

Thoughts?

cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Bruce Momjian
Bruce Momjian
bruce@momjian.us
In reply to: Andrew Dunstan (#3)
Re: Custom GUCs still a bit broken

Andrew Dunstan wrote:

Nowhere, really. I tried to fix it, but could not come up with anything
remotely clean.

So it is something for the TODO list or a 9.0 open item?

---------------------------------------------------------------------------

cheers

andrew

Bruce Momjian wrote:

Where are we on this?

---------------------------------------------------------------------------

Andrew Dunstan wrote:

It seems like Custom GUCs are still in need of some work, as shown in my
recent email. In particular, they are not transaction safe - if a
transaction attempts to do DefineCustomFooVariable() and that
transaction aborts, the placeholder setting that it used is already gone
by the time it tries to roll back GUC settings. I think this code at the
end of define_custom_variable()

/*
* Free up as much as we conveniently can of the placeholder
structure
* (this neglects any stack items...)
*/
set_string_field(pHolder, pHolder->variable, NULL);
set_string_field(pHolder, &pHolder->reset_val, NULL);

free(pHolder);

needs to be removed and instead we need to save pHolder in a list along
with the GUC level, to be processed later by AtEOXact_GUC(), which would
do the right thing according to whether or not it had a commit or an abort.

I want to get this fixed before we consider custom settings for plperl
that have possible security implications.

Thoughts?

cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do

#5Andrew Dunstan
Andrew Dunstan
andrew@dunslane.net
In reply to: Bruce Momjian (#4)
Re: Custom GUCs still a bit broken

Bruce Momjian wrote:

Andrew Dunstan wrote:

Nowhere, really. I tried to fix it, but could not come up with anything
remotely clean.

So it is something for the TODO list or a 9.0 open item?

It's not new, AFAIK. So arguably fixing it could just be a TODO. I don't
have time right now to go down that rathole.

cheers

andrew

#6Bruce Momjian
Bruce Momjian
bruce@momjian.us
In reply to: Andrew Dunstan (#5)
Re: Custom GUCs still a bit broken

Andrew Dunstan wrote:

Bruce Momjian wrote:

Andrew Dunstan wrote:

Nowhere, really. I tried to fix it, but could not come up with anything
remotely clean.

So it is something for the TODO list or a 9.0 open item?

It's not new, AFAIK. So arguably fixing it could just be a TODO. I don't
have time right now to go down that rathole.

OK, added to TODO:

Have custom GUCs be transaction safe

http://archives.postgresql.org/message-by-id.php?4B577E9F.8000505@dunslane.net

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do