Horton

Thread

Commits

  1. Restrict the use of temporary namespace in two-phase transactions

  1. Is temporary functions feature official/supported? Found some issues with it.

    Alexey Bashtanov <bashtanov@imap.cc> — 2019-01-02T17:33:12Z

    Hello,

Is creating functions in pg_temp schema something we support?
It's undocumented as far as I could see, but maybe I missed it.
People seem to use it.

I found that a combination of temporary functions and prepared 
transactions can lead to:
1) all other sessions being unable to create a temporary table until the 
prepared transaction is finished (reproducible);
2) data corruption in pg_namespace, server crash (happened a few times, 
but I'm not yet sure how to reproduce).

Is it something worth reporting in more detail?

Best,
   Alex

  2. Re: Is temporary functions feature official/supported? Found some issues with it.

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-02T19:22:29Z

    Alexey Bashtanov <bashtanov@imap.cc> writes:
> Is creating functions in pg_temp schema something we support?

Yeah.

> I found that a combination of temporary functions and prepared 
> transactions can lead to:
> 1) all other sessions being unable to create a temporary table until the 
> prepared transaction is finished (reproducible);

Locking issue perhaps?  It's not a bug if a prepared transaction
is holding a lock.

> 2) data corruption in pg_namespace, server crash (happened a few times, 
> but I'm not yet sure how to reproduce).

That would be interesting.

			regards, tom lane

  3. Re: Is temporary functions feature official/supported? Found some issues with it.

    Alexey Bashtanov <bashtanov@imap.cc> — 2019-01-02T21:55:21Z

    >> 1) all other sessions being unable to create a temporary table until the
>> prepared transaction is finished (reproducible);
> Locking issue perhaps?

For sure.

session1:

l=> begin;
BEGIN
l=> create function pg_temp.foo() returns void as $$ begin end; $$ 
language plpgsql;
CREATE FUNCTION
l=> prepare transaction 'z';
PREPARE TRANSACTION
l=> \q

session2:

l=> create temp table t();
^CCancel request sent
... blabla when inserting (0,15) into "pg_namespace_nspname_index" ... 
(sorry, server was running in non-english locale)


reprocuced in master and in 10.5

>    It's not a bug if a prepared transaction
> is holding a lock.

The weird thing is the lock comes on namespace level locking against 
creating any temporary object.
It isn't the case for without prepared transactions, neither can it be 
achieved with CREATE TEMP TABLE only with no functions involved.


>> 2) data corruption in pg_namespace, server crash (happened a few times,
>> but I'm not yet sure how to reproduce).
> That would be interesting.

I cannot reproduce it on 10.5.

Will try on master later and get back to you if I have any luck.

Best,
   Alex

  4. Re: Is temporary functions feature official/supported? Found some issues with it.

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-02T22:18:08Z

    Alexey Bashtanov <bashtanov@imap.cc> writes:
> session1:

> l=> begin;
> BEGIN
> l=> create function pg_temp.foo() returns void as $$ begin end; $$ 
> language plpgsql;
> CREATE FUNCTION
> l=> prepare transaction 'z';
> PREPARE TRANSACTION
> l=> \q

> session2:

> l=> create temp table t();
> ^CCancel request sent
> ... blabla when inserting (0,15) into "pg_namespace_nspname_index" ... 
> (sorry, server was running in non-english locale)

Hm.  I can reproduce this if I start in a virgin database, but not
otherwise.  I think the problem is that the prepared transaction has
created the pg_temp_NN schema for its session, and therefore there
is an uncommitted pg_namespace entry for that schema name, and the
second session is also trying to create that schema (because it has
the same backend ID) so it blocks waiting to see if that index
entry commits.

Another problem is that if the pg_temp_NN schema did already exist
the original session gets hung up on exit, trying to delete the
uncommitted function from its temp schema.  (This prevents other
sessions from acquiring the same backend ID, thus masking the
problem from casual inspection.)

None of this is specific to functions, it'd happen for any temp
object.

Maybe we ought to forbid prepared transactions from creating (or
deleting?) any temp objects.  I seem to remember that we already
made some restrictions of that sort, but they clearly weren't
sufficient to prevent all problems.

Or we could just say "if it hurts, don't do that".  The whole thing
is only a problem if you leave prepared transactions sitting around
for a long time, and that's already a bad idea.

> It isn't the case for without prepared transactions, neither can it be 
> achieved with CREATE TEMP TABLE only with no functions involved.

I think your experiments likely didn't account for the different
behavior depending on whether pg_temp_NN exists yet.  There's no
reason this would be special to functions.

			regards, tom lane

  5. Re: Is temporary functions feature official/supported? Found some issues with it.

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-01-02T22:54:03Z

    On 2019-Jan-02, Tom Lane wrote:

> Maybe we ought to forbid prepared transactions from creating (or
> deleting?) any temp objects.  I seem to remember that we already
> made some restrictions of that sort, but they clearly weren't
> sufficient to prevent all problems.

We make that check at transaction prepare time, but obviously there's no
way to do it any earlier since we don't know ahead of time whether the
transaction is going to do the normal commit/abort or become prepared.
Moreover, Dimitri has recently posted a patch[1] allowing prepared
transactions to commit if their temp tables are ON COMMIT DROP, which
conflicts with this approach.  This seems problematic if the pg_temp_NN
entry is reused.

Maybe we should make temp namespace names more unique if we want to
add extra features :-(

[1] https://postgr.es/m/m2d0pllvqy.fsf@dimitris-macbook-pro.home

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  6. Re: Is temporary functions feature official/supported? Found some issues with it.

    Alexey Bashtanov <bashtanov@imap.cc> — 2019-01-04T13:13:15Z

    > Hm.  I can reproduce this if I start in a virgin database, but not
> otherwise.
for a database with a history of temporary namespaces we just need more 
parallel prepared transactions

>  There's no reason this would be special to functions.
temp functions are allowed in prepared transactions, unlike tables/views 
-- that's the reason I thought about functions

Unfortunately I failed to reproduce a crash or a data corruption.

The most interesting I could see was the following:

--------example 1-------
-- session 1:
begin;
create function pg_temp.foo() returns void as $$begin end;$$ language 
plpgsql;
prepare transaction 'z';
create function pg_temp.foo() returns void as $$begin end;$$ language 
plpgsql;

-- session 2:
commit prepared 'z';

-- session 1 prints:
-- ERROR:  duplicate key value violates unique constraint 
"pg_proc_proname_args_nsp_index"
-- DETAIL:  Key (proname, proargtypes, pronamespace)=(foo, , 16385) 
already exists.

--------example 2-------
-- session 1:
begin;
create function pg_temp.foo() returns void as $$begin end;$$ language 
plpgsql;

-- session 2:
begin;
create function pg_temp.foo() returns void as $$begin end;$$ language 
plpgsql;

-- session 1:
prepare transaction 'z';
\q

-- session 2:
prepare transaction 'y';
\q

-- session 3:
begin;
create temp table t();

-- session 4:
commit prepared 'z';
commit prepared 'y';

-- session 3 prints:
-- ERROR:  duplicate key value violates unique constraint 
"pg_namespace_nspname_index"
-- LINE 1: create temp table t();
--                           ^
-- DETAIL:  Key (nspname)=(pg_temp_3) already exists.


Best,
   Alex

  7. Re: Is temporary functions feature official/supported? Found some issues with it.

    Michael Paquier <michael@paquier.xyz> — 2019-01-04T13:13:51Z

    On Wed, Jan 02, 2019 at 05:18:08PM -0500, Tom Lane wrote:
> Hm.  I can reproduce this if I start in a virgin database, but not
> otherwise.  I think the problem is that the prepared transaction has
> created the pg_temp_NN schema for its session, and therefore there
> is an uncommitted pg_namespace entry for that schema name, and the
> second session is also trying to create that schema (because it has
> the same backend ID) so it blocks waiting to see if that index
> entry commits.

That should not be allowed to commit directly.  I think that we should
just add a new value for MyXactFlags which tracks the transaction
where the temporary namespace has been created, and generate an error
if trying to use 2PC in this case.  A separate flag looks necessary to
me so as we don't bump on "cannot PREPARE a transaction that has
operated on temporary tables" in this case.  That would be
back-patchable, and if both XACT_FLAGS_ACCESSEDTEMPREL and the new
flag are set then we just blow up on the original error message when
committing.

Opinions?
--
Michael

  8. Re: Is temporary functions feature official/supported? Found some issues with it.

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-04T14:54:39Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Wed, Jan 02, 2019 at 05:18:08PM -0500, Tom Lane wrote:
>> Hm.  I can reproduce this if I start in a virgin database, but not
>> otherwise.  I think the problem is that the prepared transaction has
>> created the pg_temp_NN schema for its session, and therefore there
>> is an uncommitted pg_namespace entry for that schema name, and the
>> second session is also trying to create that schema (because it has
>> the same backend ID) so it blocks waiting to see if that index
>> entry commits.

> That should not be allowed to commit directly.  I think that we should
> just add a new value for MyXactFlags which tracks the transaction
> where the temporary namespace has been created, and generate an error
> if trying to use 2PC in this case.  A separate flag looks necessary to
> me so as we don't bump on "cannot PREPARE a transaction that has
> operated on temporary tables" in this case.

Hm, I had forgotten that we had such an error message.  Really that
restriction needs to apply to any object in the temp namespace, not only
tables.

			regards, tom lane

  9. Re: Is temporary functions feature official/supported? Found some issues with it.

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-01-04T15:43:37Z

    On 2019-Jan-04, Michael Paquier wrote:

> That should not be allowed to commit directly.  I think that we should
> just add a new value for MyXactFlags which tracks the transaction
> where the temporary namespace has been created, and generate an error
> if trying to use 2PC in this case.

That implies that a 2PC transaction will fail if it's run in a session
for which the temp namespace doesn't previously exist.  I think it's a
fairly ugly failure mode, and one that normal testing will not catch
because it'll occur very rarely.  An app that detects this problem at
run time will have to create a random temp object, commit normally, then
re-run the 2PC transaction from the start ... a lot of code to deal with
something that shouldn't happen in the first place.

I wonder if we can somehow create the temp schema in a way that makes it
immediately visible to everyone, and not depend on the commit status of
the creating transaction -- say mark the tuple with xmin=frozenXid or
something like ugly that.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  10. Re: Is temporary functions feature official/supported? Found some issues with it.

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-04T16:19:47Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Jan-04, Michael Paquier wrote:
>> That should not be allowed to commit directly.  I think that we should
>> just add a new value for MyXactFlags which tracks the transaction
>> where the temporary namespace has been created, and generate an error
>> if trying to use 2PC in this case.

> That implies that a 2PC transaction will fail if it's run in a session
> for which the temp namespace doesn't previously exist.  I think it's a
> fairly ugly failure mode, and one that normal testing will not catch
> because it'll occur very rarely.  An app that detects this problem at
> run time will have to create a random temp object, commit normally, then
> re-run the 2PC transaction from the start ... a lot of code to deal with
> something that shouldn't happen in the first place.

> I wonder if we can somehow create the temp schema in a way that makes it
> immediately visible to everyone, and not depend on the commit status of
> the creating transaction -- say mark the tuple with xmin=frozenXid or
> something like ugly that.

That's not sufficient to solve the problem, because there are really
two issues here.  Even if the temp schema already exists, we cannot
allow a 2PC transaction to create/drop/lock objects in it, because
that will mess things up for the surrounding session, or the next
session to use the same temp schema: trying to clear out the schema
will either fail or block.

			regards, tom lane

  11. Re: Is temporary functions feature official/supported? Found some issues with it.

    Michael Paquier <michael@paquier.xyz> — 2019-01-05T00:00:37Z

    On Fri, Jan 04, 2019 at 09:54:39AM -0500, Tom Lane wrote:
> Hm, I had forgotten that we had such an error message.  Really that
> restriction needs to apply to any object in the temp namespace, not only
> tables.

The cleanest way I can think we need to set up the flag each time
InitTempTableNamespace() *could* be called.  If myTempNamespace is
valid, the routine may however not be called.  So an idea is to return
immediately from InitTempTableNamespace() if myTempNamespace is valid
instead, which would mean removing its assertion at the top.  Using a
wrapper on top of InitTempTableNamespace() is also possible but I
think that we should design things so as only one code path sets
MyXactFlags, and one good thing is that InitTempTableNamespace() is
the only code path setting myTempNamespace.

For the tests, it is possible to use "\c -" to enforce a reconnection
which would be able to discard the previous temp namespace and allow
reproducibility, however as this problem applies to any objects which
can be schema-qualified this is not necessary for all the tests.

I think I can get that worked out with a back-patchable approach,
still it looks a bit sensitive because of the creation-pending logic
which relies on the assertion at the top of InitTempTableNamespace,
which is a case we may want to handle with an extra flag for the
caller of InitTempTableNamespace(), aka "fail if myTempNamespace is
valid instead of just returning back".
--
Michael

  12. Re: Is temporary functions feature official/supported? Found some issues with it.

    Michael Paquier <michael@paquier.xyz> — 2019-01-07T02:42:11Z

    On Sat, Jan 05, 2019 at 09:00:37AM +0900, Michael Paquier wrote:
> I think I can get that worked out with a back-patchable approach,
> still it looks a bit sensitive because of the creation-pending logic
> which relies on the assertion at the top of InitTempTableNamespace,
> which is a case we may want to handle with an extra flag for the
> caller of InitTempTableNamespace(), aka "fail if myTempNamespace is
> valid instead of just returning back".

Looking at this stuff, I have been able to come up with the attached,
which introduces a new flag mode for MyXactFlags, which gets set in a
couple of code paths so as PREPARE TRANSACTIOn complains:
- When attempting a LOCK on a temporary relation.
- When dropping objects part of a temporary namespace.
- When trying to access the temporary schema of a session, where it
may be initialized (or not if already present).

I have added test cases for things I could come up with, particularly
the LOCK and DROP cases which I have thought about after hacking the
code.  I think that something among those lines should be
back-patched.

Thoughts?
--
Michael

  13. Re: Is temporary functions feature official/supported? Found some issues with it.

    Masahiko Sawada <sawada.mshk@gmail.com> — 2019-01-09T07:00:51Z

    On Mon, Jan 7, 2019 at 11:42 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Sat, Jan 05, 2019 at 09:00:37AM +0900, Michael Paquier wrote:
> > I think I can get that worked out with a back-patchable approach,
> > still it looks a bit sensitive because of the creation-pending logic
> > which relies on the assertion at the top of InitTempTableNamespace,
> > which is a case we may want to handle with an extra flag for the
> > caller of InitTempTableNamespace(), aka "fail if myTempNamespace is
> > valid instead of just returning back".
>
> Looking at this stuff, I have been able to come up with the attached,
> which introduces a new flag mode for MyXactFlags, which gets set in a
> couple of code paths so as PREPARE TRANSACTIOn complains:
> - When attempting a LOCK on a temporary relation.
> - When dropping objects part of a temporary namespace.
> - When trying to access the temporary schema of a session, where it
> may be initialized (or not if already present).
>

Thank you for the patch. I've looked at the patch and have one question.

 /*
  * XACT_FLAGS_ACCESSEDTEMPREL - set when a temporary relation is accessed. We
  * don't allow PREPARE TRANSACTION in that case.
  */
 #define XACT_FLAGS_ACCESSEDTEMPREL              (1U << 0)
(snip)
 +/*
+ * XACT_FLAGS_ACCESSEDTEMPNAMESPACE - set when a temporary namespace is
+ * accessed.  We don't allow PREPARE TRANSACTION in that case.
+ */
+#define XACT_FLAGS_ACCESSEDTEMPNAMESPACE        (1U << 2)

The cases where we set XACT_FLAGS_ACCESSEDTEMPNAMESPACE seems to
include the cases setting XACT_FLAGS_ACCESSEDTEMPREL. Is there any
path where we access a temporary relation without accessing temprary
namespace?

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

  14. Re: Is temporary functions feature official/supported? Found some issues with it.

    Michael Paquier <michael@paquier.xyz> — 2019-01-09T07:30:11Z

    On Wed, Jan 09, 2019 at 04:00:51PM +0900, Masahiko Sawada wrote:
>  /*
>   * XACT_FLAGS_ACCESSEDTEMPREL - set when a temporary relation is accessed. We
>   * don't allow PREPARE TRANSACTION in that case.
>   */
>  #define XACT_FLAGS_ACCESSEDTEMPREL              (1U << 0)
> (snip)
>  +/*
> + * XACT_FLAGS_ACCESSEDTEMPNAMESPACE - set when a temporary namespace is
> + * accessed.  We don't allow PREPARE TRANSACTION in that case.
> + */
> +#define XACT_FLAGS_ACCESSEDTEMPNAMESPACE        (1U << 2)
> 
> The cases where we set XACT_FLAGS_ACCESSEDTEMPNAMESPACE seems to
> include the cases setting XACT_FLAGS_ACCESSEDTEMPREL. Is there any
> path where we access a temporary relation without accessing temporary
> namespace?

This case would set ACCESSEDTEMPREL but not ACCESSEDTEMPNAMESPACE:
create temp table twophase_tab (a int);
begin;
select a from twophase_tab;
prepare transaction 'twophase_tab';

I kept the original flag mainly for compatibility with the past
handling so as the error message remains constant and back-patchable
for application relying on the existing behavior.  I think that for
HEAD we could consider moving on with an approach where we have only
ACCESSEDTEMPNAMESPACE, still we may want to make a difference for
transactions which have actually tried to take any kind of locks on
the temporary relation.
--
Michael

  15. Re: Is temporary functions feature official/supported? Found some issues with it.

    Masahiko Sawada <sawada.mshk@gmail.com> — 2019-01-10T02:47:32Z

    On Wed, Jan 9, 2019 at 4:30 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Jan 09, 2019 at 04:00:51PM +0900, Masahiko Sawada wrote:
> >  /*
> >   * XACT_FLAGS_ACCESSEDTEMPREL - set when a temporary relation is accessed. We
> >   * don't allow PREPARE TRANSACTION in that case.
> >   */
> >  #define XACT_FLAGS_ACCESSEDTEMPREL              (1U << 0)
> > (snip)
> >  +/*
> > + * XACT_FLAGS_ACCESSEDTEMPNAMESPACE - set when a temporary namespace is
> > + * accessed.  We don't allow PREPARE TRANSACTION in that case.
> > + */
> > +#define XACT_FLAGS_ACCESSEDTEMPNAMESPACE        (1U << 2)
> >
> > The cases where we set XACT_FLAGS_ACCESSEDTEMPNAMESPACE seems to
> > include the cases setting XACT_FLAGS_ACCESSEDTEMPREL. Is there any
> > path where we access a temporary relation without accessing temporary
> > namespace?
>
> This case would set ACCESSEDTEMPREL but not ACCESSEDTEMPNAMESPACE:
> create temp table twophase_tab (a int);
> begin;
> select a from twophase_tab;
> prepare transaction 'twophase_tab';

Thank you.

>
> I kept the original flag mainly for compatibility with the past
> handling so as the error message remains constant and back-patchable
> for application relying on the existing behavior.

Understood. Sounds good.

>  I think that for
> HEAD we could consider moving on with an approach where we have only
> ACCESSEDTEMPNAMESPACE, still we may want to make a difference for
> transactions which have actually tried to take any kind of locks on
> the temporary relation.

Yes, I was confused that in spite of the relation is a kind of
database object we separate two cases, relation and database objects
other than relation. So I thought that we can merge these flags to
something like XACT_FLAGS_ACCESSEDTEMPOBJ tracking whether the
transaction created, locked or dropped objects such as tables,
functions and datatypes on a temoprary namespace.

I've found a corner-case issue with this patch; this issue still
happens when we create the extension on the temprary namespace,
because it can be done without accessing the namespace if the
temporary namespace is already created.

postgres(1:59232)=# create table pg_temp.e(c int);
CREATE TABLE
postgres(1:59232)=# begin;
BEGIN
postgres(1:59232)=# create extension pgcrypto schema pg_temp_3;
CREATE EXTENSION
postgres(1:59232)=# prepare transaction 'a';
PREPARE TRANSACTION
postgres(1:59232)=# create extension pgcrypto;
(hang..)

To fix we could have get_namepace_oid set
XACT_FLAGS_ACCESSEDTEMPNAMESPACE (not calling
InitTempTableNamespace()) if the nspname is pg_temp_NNN but it could
influence many places.

Also, since even the current_schame() and the current_scheames() could
create the temporary namespace when executing fetch_search_path()
while 'pg_temp' appears the head of seach_patch, for example, the
following transaction cannot prepre. But it possible coulld be
acceptable by users as this is very corner case and it could be
regards as accessing the temporary namespace actually.

postgres(1:59025)=# set search_path to pg_temp, "$user", public;
SET
postgres(1:59025)=# begin;
BEGIN
postgres(1:59025)=# select current_schema;
 current_schema
----------------
 pg_temp_3
(1 row)
postgres(1:59025)=# prepare transaction 'a';
ERROR:  cannot PREPARE a transaction that has operated on temporary namespace


Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

  16. Re: Is temporary functions feature official/supported? Found some issues with it.

    Michael Paquier <michael@paquier.xyz> — 2019-01-10T03:45:41Z

    On Thu, Jan 10, 2019 at 11:47:32AM +0900, Masahiko Sawada wrote:
> Yes, I was confused that in spite of the relation is a kind of
> database object we separate two cases, relation and database objects
> other than relation. So I thought that we can merge these flags to
> something like XACT_FLAGS_ACCESSEDTEMPOBJ tracking whether the
> transaction created, locked or dropped objects such as tables,
> functions and datatypes on a temoprary namespace.

Sure.  For HEAD I don't mind doing so and that makes sense.  On
back-branches though that's another story... 

> I've found a corner-case issue with this patch; this issue still
> happens when we create the extension on the temprary namespace,
> because it can be done without accessing the namespace if the
> temporary namespace is already created.
> 
> postgres(1:59232)=# create table pg_temp.e(c int);
> CREATE TABLE
> postgres(1:59232)=# begin;
> BEGIN
> postgres(1:59232)=# create extension pgcrypto schema pg_temp_3;
> CREATE EXTENSION
> postgres(1:59232)=# prepare transaction 'a';
> PREPARE TRANSACTION
> postgres(1:59232)=# create extension pgcrypto;
> (hang..)

There is a reason why I have split that part into a separate patch and
a separate thread: 
https://commitfest.postgresql.org/22/1969/

The thing is that this behavior is rather unstable, for example I bump
easily into that:
=# create extension dblink schema pg_temp_3;
ERROR:  42883: function dblink_connect_u(text) does not exist

> Also, since even the current_schame() and the current_schemas() could
> create the temporary namespace when executing fetch_search_path()
> while 'pg_temp' appears the head of seach_patch, for example, the
> following transaction cannot prepre. But it possible could be
> acceptable by users as this is very corner case and it could be
> regards as accessing the temporary namespace actually.

Yeah, but the temporary schema gets created, which is something that
we want to avoid, so 2PC must fail on that case as well or you would
run again into the same locking hazards if the same temp schema gets
reused in a different backend.
--
Michael

  17. Re: Is temporary functions feature official/supported? Found some issues with it.

    Masahiko Sawada <sawada.mshk@gmail.com> — 2019-01-10T11:44:13Z

    On Thu, Jan 10, 2019 at 12:45 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Jan 10, 2019 at 11:47:32AM +0900, Masahiko Sawada wrote:
> > Yes, I was confused that in spite of the relation is a kind of
> > database object we separate two cases, relation and database objects
> > other than relation. So I thought that we can merge these flags to
> > something like XACT_FLAGS_ACCESSEDTEMPOBJ tracking whether the
> > transaction created, locked or dropped objects such as tables,
> > functions and datatypes on a temoprary namespace.
>
> Sure.  For HEAD I don't mind doing so and that makes sense.  On
> back-branches though that's another story...

Agreed.

>
> > I've found a corner-case issue with this patch; this issue still
> > happens when we create the extension on the temprary namespace,
> > because it can be done without accessing the namespace if the
> > temporary namespace is already created.
> >
> > postgres(1:59232)=# create table pg_temp.e(c int);
> > CREATE TABLE
> > postgres(1:59232)=# begin;
> > BEGIN
> > postgres(1:59232)=# create extension pgcrypto schema pg_temp_3;
> > CREATE EXTENSION
> > postgres(1:59232)=# prepare transaction 'a';
> > PREPARE TRANSACTION
> > postgres(1:59232)=# create extension pgcrypto;
> > (hang..)
>
> There is a reason why I have split that part into a separate patch and
> a separate thread:
> https://commitfest.postgresql.org/22/1969/
>
> The thing is that this behavior is rather unstable, for example I bump
> easily into that:
> =# create extension dblink schema pg_temp_3;
> ERROR:  42883: function dblink_connect_u(text) does not exist

Yes, IIUC this issue happen only when creating the extension that
doesn't access the function after created it. For example, dblink does
REVOKE for dblink_connect_u() after creation.


Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

  18. Re: Is temporary functions feature official/supported? Found some issues with it.

    Michael Paquier <michael@paquier.xyz> — 2019-01-14T00:45:20Z

    On Thu, Jan 10, 2019 at 08:44:13PM +0900, Masahiko Sawada wrote:
> Yes, IIUC this issue happen only when creating the extension that
> doesn't access the function after created it. For example, dblink does
> REVOKE for dblink_connect_u() after creation.

I am not sure if Robert is a fan of simply forbiding that as per the
question here:
https://www.postgresql.org/message-id/CA+TgmoZJdGcGFm+coHHCbBM3CfPqpjisdiux4Pa9QA3dFFQasw@mail.gmail.com

Anyway, we could take for now the separate approach to prevent the
case of CREATE EXTENSION with 2PC if trying to use a temporary
schema as more holes are closed with this stuff.  Attached is an
updated patch to do so.  This adds a regression test, which would fail
if we decide to prevent the behavior afterwards. This uses two tricks
to avoid CONTEXT and NOTICE messages which include the temporary
session name to keep the test stable:
\set SHOW_CONTEXT never
SET client_min_messages TO 'warning';

Thoughts?
--
Michael

  19. Re: Is temporary functions feature official/supported? Found some issues with it.

    Masahiko Sawada <sawada.mshk@gmail.com> — 2019-01-15T06:36:56Z

    On Mon, Jan 14, 2019 at 9:45 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Jan 10, 2019 at 08:44:13PM +0900, Masahiko Sawada wrote:
> > Yes, IIUC this issue happen only when creating the extension that
> > doesn't access the function after created it. For example, dblink does
> > REVOKE for dblink_connect_u() after creation.
>
> I am not sure if Robert is a fan of simply forbiding that as per the
> question here:
> https://www.postgresql.org/message-id/CA+TgmoZJdGcGFm+coHHCbBM3CfPqpjisdiux4Pa9QA3dFFQasw@mail.gmail.com
>
> Anyway, we could take for now the separate approach to prevent the
> case of CREATE EXTENSION with 2PC if trying to use a temporary
> schema as more holes are closed with this stuff.  Attached is an
> updated patch to do so.  This adds a regression test, which would fail
> if we decide to prevent the behavior afterwards. This uses two tricks
> to avoid CONTEXT and NOTICE messages which include the temporary
> session name to keep the test stable:
> \set SHOW_CONTEXT never
> SET client_min_messages TO 'warning';
>
> Thoughts?

Thank you for updating the patch. The patch looks good to me. And the
new regression test for CREATE EXTENSION seems to work fine but maybe
it's better to reset client_min_messages at cleanup for safety.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

  20. Re: Is temporary functions feature official/supported? Found some issues with it.

    Michael Paquier <michael@paquier.xyz> — 2019-01-16T01:36:33Z

    On Tue, Jan 15, 2019 at 03:36:56PM +0900, Masahiko Sawada wrote:
> Thank you for updating the patch. The patch looks good to me. And the
> new regression test for CREATE EXTENSION seems to work fine but maybe
> it's better to reset client_min_messages at cleanup for safety.

Sure, done.  I have been working on this patch more this morning, and
here is a proposal for commit.  So please let me know if there are any
objections with that.

9b013dc is the commit which has introduced MyXactFlags, so this means
that we cannot get that back-patched further down than v10.  Per the
lack of complains, that's a restriction I can live with.

The first patch is the actual fix to back-patch.  The second patch is
an improvement I propose only for HEAD which removes ACCESSEDTEMPREL,
replacing it with ACCESSEDTEMPNAMESPACE.  For now I propose to commit
0001, then I'll spawn a new thread to discuss 0002 on -hackers as
XACT_FLAGS_ACCESSEDTEMPREL has the advantage to allow skipping ON
COMMIT DELETE if no temporary tables have been accessed.  Perhaps
that's not worth bothering, but that point seems worth poking at, at
least to me.
--
Michael

  21. Re: Is temporary functions feature official/supported? Found some issues with it.

    Michael Paquier <michael@paquier.xyz> — 2019-01-18T00:28:32Z

    On Wed, Jan 16, 2019 at 10:36:33AM +0900, Michael Paquier wrote:
> Sure, done.  I have been working on this patch more this morning, and
> here is a proposal for commit.  So please let me know if there are any
> objections with that.

Committed down to v10 which is where MyXactFlags is available.
--
Michael

  22. Re: Is temporary functions feature official/supported? Found some issues with it.

    Masahiko Sawada <sawada.mshk@gmail.com> — 2019-01-18T02:37:14Z

    On Fri, Jan 18, 2019 at 9:28 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Jan 16, 2019 at 10:36:33AM +0900, Michael Paquier wrote:
> > Sure, done.  I have been working on this patch more this morning, and
> > here is a proposal for commit.  So please let me know if there are any
> > objections with that.
>
> Committed down to v10 which is where MyXactFlags is available.

Thank you!

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center