BUG #14825: enum type: unsafe use?

balazs@obiserver.hu writes:

PostgreSQL version: 10beta4

testdb=# begin;
BEGIN
testdb=# create type enumtype as enum ('v1', 'v2');
CREATE TYPE
testdb=# alter type enumtype owner to testrole;
ALTER TYPE
testdb=# create table testtable (enumcolumn enumtype not null default 'v1');
ERROR: unsafe use of new value "v1" of enum type enumtype
HINT: New enum values must be committed before they can be used.

Hmm, that's pretty annoying. It's happening because check_safe_enum_use
insists that the enum's pg_type entry not be updated-in-transaction.
We thought that the new rules instituted by 15bc038f9 would be generally
more convenient to use than the old ones --- but this example shows
that, for example, pg_dump scripts involving enums often could not
be restored inside a single transaction.

I'm not sure if that qualifies as a stop-ship problem, but it ain't
good, for sure. We need to look at whether we should revert 15bc038f9
or somehow revise its rules.

regards, tom lane

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

On 09/22/2017 05:46 PM, Tom Lane wrote:

balazs@obiserver.hu writes:

PostgreSQL version: 10beta4
testdb=# begin;
BEGIN
testdb=# create type enumtype as enum ('v1', 'v2');
CREATE TYPE
testdb=# alter type enumtype owner to testrole;
ALTER TYPE
testdb=# create table testtable (enumcolumn enumtype not null default 'v1');
ERROR: unsafe use of new value "v1" of enum type enumtype
HINT: New enum values must be committed before they can be used.

Hmm, that's pretty annoying. It's happening be

cause check_safe_enum_use
insists that the enum's pg_type entry not be updated-in-transaction.
We thought that the new rules instituted by 15bc038f9 would be generally
more convenient to use than the old ones --- but this example shows
that, for example, pg_dump scripts involving enums often could not
be restored inside a single transaction.

I'm not sure if that qualifies as a stop-ship problem, but it ain't
good, for sure. We need to look at whether we should revert 15bc038f9
or somehow revise its rules.

:-(

The only real problem comes from adding a value to an enum that has been
created in an earlier transaction and then using that enum value. What
we're doing here is essentially a heuristic test for that condition, and
we're getting some false positives. I wonder if we wouldn't be better
doing this more directly, keeping a per-transaction hash of unsafe enum
values (which will almost always be empty). It might even speed up the
check.

cheers

andrew

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

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

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 09/22/2017 05:46 PM, Tom Lane wrote:

I'm not sure if that qualifies as a stop-ship problem, but it ain't
good, for sure. We need to look at whether we should revert 15bc038f9
or somehow revise its rules.

I wonder if we wouldn't be better
doing this more directly, keeping a per-transaction hash of unsafe enum
values (which will almost always be empty). It might even speed up the
check.

Yeah, I was considering the same thing over dinner, though I'd phrase
it oppositely: keep a list of enum type OIDs created in the current
transaction, so that we could whitelist them. This could maybe become
a problem if someone created a zillion enums in one xact, though.

The immediate question is do we care to design/implement such a thing
post-RC1. I'd have to vote "no". I think the most prudent thing to
do is revert 15bc038f9 and then have another go at it during the v11
cycle.

regards, tom lane

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

On 09/22/2017 11:19 PM, Tom Lane wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 09/22/2017 05:46 PM, Tom Lane wrote:

I'm not sure if that qualifies as a stop-ship problem, but it ain't
good, for sure. We need to look at whether we should revert 15bc038f9
or somehow revise its rules.

I wonder if we wouldn't be better
doing this more directly, keeping a per-transaction hash of unsafe enum
values (which will almost always be empty). It might even speed up the
check.

Yeah, I was considering the same thing over dinner, though I'd phrase
it oppositely: keep a list of enum type OIDs created in the current
transaction, so that we could whitelist them. This could maybe become
a problem if someone created a zillion enums in one xact, though.

I see what you're saying, but my idea was slightly different. We would
only add to the hashtable I had in mind at the bottom AddEnumLabel().
Any other value, whether added in the current transaction or not, should
be safe, AIUI. Maybe we should also keep a cache of whitelisted enums
created in the current transaction.

I'm not to worried about people creating a zillion enums (or enum labels
being added for the solution I had in mind). Even a hash of a million
Oids will only consume a few megabytes, won't it?

The immediate question is do we care to design/implement such a thing
post-RC1. I'd have to vote "no". I think the most prudent thing to
do is revert 15bc038f9 and then have another go at it during the v11
cycle.

Sadly I agree. We've made decisions like this in the past, and I have
generally been supportive of them. I think this is the first time I have
been on the receiving end of one so late in the process :-(

cheers

andrew

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

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

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 09/22/2017 11:19 PM, Tom Lane wrote:

Yeah, I was considering the same thing over dinner, though I'd phrase
it oppositely: keep a list of enum type OIDs created in the current
transaction, so that we could whitelist them. This could maybe become
a problem if someone created a zillion enums in one xact, though.

I see what you're saying, but my idea was slightly different. We would
only add to the hashtable I had in mind at the bottom AddEnumLabel().
Any other value, whether added in the current transaction or not, should
be safe, AIUI.

Oh, I see: a list of enum values we need to blacklist, not whitelist.
Yes, that's a significantly better idea than mine, because in common
use-cases that would be empty or have a very small number of entries.
In particular that fixes the "pg_restore -1" scenario, because no
matter how many enums you're restoring, pg_dump doesn't use ALTER
TYPE ADD VALUE. (Well, it does in --binary-upgrade mode, but those
scripts are run in transaction-per-statement mode so it's fine.)

Maybe we should also keep a cache of whitelisted enums
created in the current transaction.

What for? Wouldn't be any faster to search, in fact likely slower
because it could get large in common use-cases.

The immediate question is do we care to design/implement such a thing
post-RC1. I'd have to vote "no". I think the most prudent thing to
do is revert 15bc038f9 and then have another go at it during the v11
cycle.

Sadly I agree. We've made decisions like this in the past, and I have
generally been supportive of them. I think this is the first time I have
been on the receiving end of one so late in the process :-(

Unless you want to try writing a patch for this in the next day or two,
I think we have to do that. But now that I see the plan clearly, maybe
we could get away with a post-RC1 fix.

regards, tom lane

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

On 09/23/2017 11:16 AM, Tom Lane wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

The immediate question is do we care to design/implement such a thing
post-RC1. I'd have to vote "no". I think the most prudent thing to
do is revert 15bc038f9 and then have another go at it during the v11
cycle.

Sadly I agree. We've made decisions like this in the past, and I have
generally been supportive of them. I think this is the first time I have
been on the receiving end of one so late in the process :-(

Unless you want to try writing a patch for this in the next day or two,
I think we have to do that. But now that I see the plan clearly, maybe
we could get away with a post-RC1 fix.

OK, I'll give it a shot.

cheers

andrew

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

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

I wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

I see what you're saying, but my idea was slightly different. We would
only add to the hashtable I had in mind at the bottom AddEnumLabel().
Any other value, whether added in the current transaction or not, should
be safe, AIUI.

Oh, I see: a list of enum values we need to blacklist, not whitelist.
Yes, that's a significantly better idea than mine, because in common
use-cases that would be empty or have a very small number of entries.

Oh, wait a minute ... that's not where the problem is, really. We can
already tell reliably whether an enum value was created in the current
transaction: the is-it-committed check in check_safe_enum_use is
perfectly safe for that AFAICS. The hard part of this is the part about
"was the enum type created in the current transaction?". We could make
that reliable with the table I proposed of enum types created in the
current transaction, but the possible performance drag is a concern.

What I understand you to be proposing is to blacklist individual
enum values added by ALTER ADD VALUE, but *not* values created
aboriginally by CREATE TYPE AS ENUM. (The latter are surely safe,
because the type must disappear if they do.) However, that would
require dropping the second part of the current documentation promise:

If <command>ALTER TYPE ... ADD VALUE</> (the form that adds a new value to
an enum type) is executed inside a transaction block, the new value cannot
be used until after the transaction has been committed, except in the case
that the enum type itself was created earlier in the same transaction.

We'd have to just say "it can't be used till the transaction has been
committed", full stop. Otherwise we're right back into the problem of
needing to detect whether the enum type is new in transaction.

Maybe we should also keep a cache of whitelisted enums
created in the current transaction.

What for?

I now realize that what you probably meant here was to track enum *types*,
not values, created in the current transaction. But if we're doing that
then we don't really need the per-enum-value-blacklist part of it.

So I'm back to not being sure about the path forward. Maybe it would be
all right to say "the value added by ADD VALUE can't be used in the same
transaction, period". That's still a step forward compared to the pre-v10
prohibition on doing it at all. I don't remember if there were use-cases
where we really needed the exception for new-in-transaction types.

regards, tom lane

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

On 09/23/2017 02:00 PM, Tom Lane wrote:

I wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

I see what you're saying, but my idea was slightly different. We would
only add to the hashtable I had in mind at the bottom AddEnumLabel().
Any other value, whether added in the current transaction or not, should
be safe, AIUI.

Oh, I see: a list of enum values we need to blacklist, not whitelist.
Yes, that's a significantly better idea than mine, because in common
use-cases that would be empty or have a very small number of entries.

Oh, wait a minute ... that's not where the problem is, really. We can
already tell reliably whether an enum value was created in the current
transaction: the is-it-committed check in check_safe_enum_use is
perfectly safe for that AFAICS. The hard part of this is the part about
"was the enum type created in the current transaction?". We could make
that reliable with the table I proposed of enum types created in the
current transaction, but the possible performance drag is a concern.

What I understand you to be proposing is to blacklist individual
enum values added by ALTER ADD VALUE, but *not* values created
aboriginally by CREATE TYPE AS ENUM. (The latter are surely safe,
because the type must disappear if they do.) However, that would
require dropping the second part of the current documentation promise:

If <command>ALTER TYPE ... ADD VALUE</> (the form that adds a new value to
an enum type) is executed inside a transaction block, the new value cannot
be used until after the transaction has been committed, except in the case
that the enum type itself was created earlier in the same transaction.

We'd have to just say "it can't be used till the transaction has been
committed", full stop. Otherwise we're right back into the problem of
needing to detect whether the enum type is new in transaction.

Maybe we should also keep a cache of whitelisted enums
created in the current transaction.

What for?

I now realize that what you probably meant here was to track enum *types*,
not values, created in the current transaction. But if we're doing that
then we don't really need the per-enum-value-blacklist part of it.

So I'm back to not being sure about the path forward. Maybe it would be
all right to say "the value added by ADD VALUE can't be used in the same
transaction, period". That's still a step forward compared to the pre-v10
prohibition on doing it at all. I don't remember if there were use-cases
where we really needed the exception for new-in-transaction types.

Well, my idea was to have the test run like this:

* is the value an old one? Test txnid of tuple. If yes it's ok
* is the value one created by ALTER TYPE ADD VALUE? Test
blacklist. If no, it's ok.
* is the enum a new one? Test whitelist. If yes, it's ok.
* anything else is not ok.

I think that would let us keep our promise.

If we just did the blacklist and stuck with our current heuristic test
for enum being created in the current transaction, we'd still probably
avoid 99% of the problems, including specifically the one that gave rise
to the bug report.

Am I missing something?

cheers

andrew

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

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

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 09/23/2017 02:00 PM, Tom Lane wrote:

So I'm back to not being sure about the path forward. Maybe it would be
all right to say "the value added by ADD VALUE can't be used in the same
transaction, period". That's still a step forward compared to the pre-v10
prohibition on doing it at all. I don't remember if there were use-cases
where we really needed the exception for new-in-transaction types.

Well, my idea was to have the test run like this:

* is the value an old one? Test txnid of tuple. If yes it's ok
* is the value one created by ALTER TYPE ADD VALUE? Test
blacklist. If no, it's ok.
* is the enum a new one? Test whitelist. If yes, it's ok.
* anything else is not ok.

My point is that if you do 1 and 3, you don't need 2. Or if you do
2 and 3, you don't need 1. But in most cases, testing the tuple
hint bits is cheap, so you don't really want that option.

In any case, what I'm worried about is the amount of bookkeeping
overhead added by keeping a whitelist of enum-types-created-in-
current-transaction. That's less than trivial, especially since
you have to account correctly for subtransactions. And there are
common use-cases where that table will become large.

If we just did the blacklist and stuck with our current heuristic test
for enum being created in the current transaction, we'd still probably
avoid 99% of the problems, including specifically the one that gave rise
to the bug report.

True. But I'm not sure whether the heuristic test is adding anything
meaningful if we use a blacklist first. The case where it could help
is

begin;
create type t as enum();
alter type t add value 'v';
-- do something with 'v'
commit;

That perhaps is worth something, but if somebody is trying to build a new
enum type in pieces like that, doesn't it seem fairly likely that they
might throw in an ALTER OWNER or GRANT as well? My feeling is that the
lesson we need to learn is that the heuristic test isn't good enough.

regards, tom lane

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

On 09/23/2017 03:52 PM, Tom Lane wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 09/23/2017 02:00 PM, Tom Lane wrote:

So I'm back to not being sure about the path forward. Maybe it would be
all right to say "the value added by ADD VALUE can't be used in the same
transaction, period". That's still a step forward compared to the pre-v10
prohibition on doing it at all. I don't remember if there were use-cases
where we really needed the exception for new-in-transaction types.

Well, my idea was to have the test run like this:
* is the value an old one? Test txnid of tuple. If yes it's ok
* is the value one created by ALTER TYPE ADD VALUE? Test
blacklist. If no, it's ok.
* is the enum a new one? Test whitelist. If yes, it's ok.
* anything else is not ok.

My point is that if you do 1 and 3, you don't need 2. Or if you do
2 and 3, you don't need 1. But in most cases, testing the tuple
hint bits is cheap, so you don't really want that option.

In any case, what I'm worried about is the amount of bookkeeping
overhead added by keeping a whitelist of enum-types-created-in-
current-transaction. That's less than trivial, especially since
you have to account correctly for subtransactions. And there are
common use-cases where that table will become large.

If we just did the blacklist and stuck with our current heuristic test
for enum being created in the current transaction, we'd still probably
avoid 99% of the problems, including specifically the one that gave rise
to the bug report.

True. But I'm not sure whether the heuristic test is adding anything
meaningful if we use a blacklist first. The case where it could help
is

begin;
create type t as enum();
alter type t add value 'v';
-- do something with 'v'
commit;

That perhaps is worth something, but if somebody is trying to build a new
enum type in pieces like that, doesn't it seem fairly likely that they
might throw in an ALTER OWNER or GRANT as well? My feeling is that the
lesson we need to learn is that the heuristic test isn't good enough.

OK, I think I'm convinced. Here's is the WIP code I put together for the
blacklist. I'm was looking for a place to put the init call, but since
it's possibly not going anywhere I stopped :-) . My initial thought
about substransactions was that we should ignore them for this purpose
(That's why I used TopTransactionContext for the table).

I agree the heuristic test isn't good enough, and if we can get a 100%
accurate test for the newness of the enum type then the blacklist would
be redundant.

w.r.t. table size - how large? I confess I haven't seen any systems with
more than a few hundred enum types. But even a million or two shouldn't
consume a huge amount of memory, should it?

cheers

andrew

--

Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

OK, I think I'm convinced. Here's is the WIP code I put together for the
blacklist. I'm was looking for a place to put the init call, but since
it's possibly not going anywhere I stopped :-) . My initial thought
about substransactions was that we should ignore them for this purpose
(That's why I used TopTransactionContext for the table).

For the blacklist, I agree we could just ignore subtransactions: all
subtransaction levels are equally uncommitted for this purpose, and
leaving entries from failed subtransactions in place seems like a
non-issue, since they'd never be referenced again. (Well, barring OID
wraparound and an enum-value-OID collision while the transaction runs,
but I think we can ignore that as having probability epsilon.)

But you need to actually put the table in TopTransactionContext, not
CurTransactionContext ;-). Also, I don't think you need an init call
so much as an end-of-transaction cleanup call. Maybe call it
AtEOXactEnum(), for consistency with other functions called in the
same area.

w.r.t. table size - how large? I confess I haven't seen any systems with
more than a few hundred enum types. But even a million or two shouldn't
consume a huge amount of memory, should it?

Dynahash tables are self-expanding, so I don't see a need to stress about
that too much. Anything in 10-100 seems reasonable for initial size.

regards, tom lane

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

On 09/23/2017 06:06 PM, Tom Lane wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

OK, I think I'm convinced. Here's is the WIP code I put together for the
blacklist. I'm was looking for a place to put the init call, but since
it's possibly not going anywhere I stopped :-) . My initial thought
about substransactions was that we should ignore them for this purpose
(That's why I used TopTransactionContext for the table).

For the blacklist, I agree we could just ignore subtransactions: all
subtransaction levels are equally uncommitted for this purpose, and
leaving entries from failed subtransactions in place seems like a
non-issue, since they'd never be referenced again. (Well, barring OID
wraparound and an enum-value-OID collision while the transaction runs,
but I think we can ignore that as having probability epsilon.)

But you need to actually put the table in TopTransactionContext, not
CurTransactionContext ;-). Also, I don't think you need an init call
so much as an end-of-transaction cleanup call. Maybe call it
AtEOXactEnum(), for consistency with other functions called in the
same area.

w.r.t. table size - how large? I confess I haven't seen any systems with
more than a few hundred enum types. But even a million or two shouldn't
consume a huge amount of memory, should it?

Dynahash tables are self-expanding, so I don't see a need to stress about
that too much. Anything in 10-100 seems reasonable for initial size.

OK, here's the finished patch. It has a pretty small footprint all
things considered, and I think it guarantees that nothing that could be
done in this area in 9.6 will be forbidden. That's probably enough to
get us to 10 without having to revert the whole thing, ISTM, and we can
leave any further refinement to the next release.

cheers

andrew

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

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

OK, here's the finished patch. It has a pretty small footprint all
things considered, and I think it guarantees that nothing that could be
done in this area in 9.6 will be forbidden. That's probably enough to
get us to 10 without having to revert the whole thing, ISTM, and we can
leave any further refinement to the next release.

I think this could do with some more work on the comments and test cases,
but it's basically sound.

What we still need to debate is whether to remove the heuristic
type-is-from-same-transaction test, making the user-visible behavior
simply "you must commit an ALTER TYPE ADD VALUE before you can use the
new value". I'm kind of inclined to do so; the fuzzy (and inadequately
documented) behavior we'll have if we keep it doesn't seem very nice to
me.

regards, tom lane

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

On 09/24/2017 04:37 PM, Tom Lane wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

OK, here's the finished patch. It has a pretty small footprint all
things considered, and I think it guarantees that nothing that could be
done in this area in 9.6 will be forbidden. That's probably enough to
get us to 10 without having to revert the whole thing, ISTM, and we can
leave any further refinement to the next release.

I think this could do with some more work on the comments and test cases,
but it's basically sound.

What we still need to debate is whether to remove the heuristic
type-is-from-same-transaction test, making the user-visible behavior
simply "you must commit an ALTER TYPE ADD VALUE before you can use the
new value". I'm kind of inclined to do so; the fuzzy (and inadequately
documented) behavior we'll have if we keep it doesn't seem very nice to
me.

I'd rather not. The failure cases are going to be vanishingly small, I
suspect, and we've already discussed how we might improve that test. If
you want to put some weasel words in the docs that might be ok.

cheers

andrew

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

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

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 09/24/2017 04:37 PM, Tom Lane wrote:

What we still need to debate is whether to remove the heuristic
type-is-from-same-transaction test, making the user-visible behavior
simply "you must commit an ALTER TYPE ADD VALUE before you can use the
new value". I'm kind of inclined to do so; the fuzzy (and inadequately
documented) behavior we'll have if we keep it doesn't seem very nice to
me.

I'd rather not. The failure cases are going to be vanishingly small, I
suspect, and we've already discussed how we might improve that test. If
you want to put some weasel words in the docs that might be ok.

I'm unconvinced.  We get enough complaints about heuristic behaviors
we have elsewhere.  Also, if we ship it like this, we're going to
have backward compatibility concerns if we try to change the behavior
later.  Now admittedly, the next step forward might well be an exact
solution which would necessarily take every case the heuristic allows
--- but I don't want to box us into having to support exactly the
cases the heuristic would allow.  And I don't want to have to
document which those are, either.

Basically, I don't think anyone's shown an important use case that
wouldn't be covered by "committed or not blacklisted". That fixes
the original complaint that you couldn't do ALTER ADD VALUE in a
transaction block at all, and with or without the heuristic test,
you can't use the added value without committing. The case not
covered is where an enum type is built with multiple commands in a
single transaction --- which might be of value, but since it doesn't
work for every such case, we don't know if the heuristic is really
going to provide useful value-add or not.

So I think we should just stop with the blacklist test for v10,
and then see if we still get complaints (and exactly what they're
about) so that we can judge how much more work the problem deserves.
It's still ahead of where we were in previous releases, and ahead of
where we'd be if we end up reverting the patch altogether.

Or in short: having been burned by this heuristic already, I want
it out of there.

regards, tom lane

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

On 09/24/2017 07:06 PM, Tom Lane wrote:

So I think we should just stop with the blacklist test for v10,
and then see if we still get complaints (and exactly what they're
about) so that we can judge how much more work the problem deserves.
It's still ahead of where we were in previous releases, and ahead of
where we'd be if we end up reverting the patch altogether.

That's pretty much what I was saying.

cheers

andrew

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

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

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 09/24/2017 07:06 PM, Tom Lane wrote:

So I think we should just stop with the blacklist test for v10,
and then see if we still get complaints (and exactly what they're
about) so that we can judge how much more work the problem deserves.
It's still ahead of where we were in previous releases, and ahead of
where we'd be if we end up reverting the patch altogether.

That's pretty much what I was saying.

Oh ... I did not think we were on the same page, because your patch
didn't include removal of the same-transaction heuristic. It'd be
sensible to do that as a separate patch, though, to make it easier
to put back if we decide we do want it.

regards, tom lane

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

On 09/25/2017 10:14 AM, Tom Lane wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 09/24/2017 07:06 PM, Tom Lane wrote:

So I think we should just stop with the blacklist test for v10,
and then see if we still get complaints (and exactly what they're
about) so that we can judge how much more work the problem deserves.
It's still ahead of where we were in previous releases, and ahead of
where we'd be if we end up reverting the patch altogether.

That's pretty much what I was saying.

Oh ... I did not think we were on the same page, because your patch
didn't include removal of the same-transaction heuristic. It'd be
sensible to do that as a separate patch, though, to make it easier
to put back if we decide we do want it.

I understood you to say that the blacklist patch was all we needed to do
for v10. That's my position, i.e. I think we can live with the heuristic
test for now if the blacklist patch is applied. Maybe we need to
document that the heuristic test can generate some false negatives when
testing for a type that is created in the current transaction.

cheers

andrew

--

Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 09/25/2017 10:14 AM, Tom Lane wrote:

Oh ... I did not think we were on the same page, because your patch
didn't include removal of the same-transaction heuristic. It'd be
sensible to do that as a separate patch, though, to make it easier
to put back if we decide we do want it.

I understood you to say that the blacklist patch was all we needed to do
for v10. That's my position, i.e. I think we can live with the heuristic
test for now if the blacklist patch is applied. Maybe we need to
document that the heuristic test can generate some false negatives when
testing for a type that is created in the current transaction.

No, as I said upthread, I want the heuristic out of there. I think the
blacklist idea covers enough use-cases that we possibly don't need the
same-transaction test at all. Furthermore I'm doubtful that the heuristic
form of the same-transaction test is adequate to satisfy the use-cases
that the blacklist test doesn't cover. So I think we should remove that
test and see whether we get any complaints, and if so what the details of
the real-world use-cases look like.

regards, tom lane

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