Re: LOCK for non-tables

One of the things that I ripped out of the SQL/MED syntax patch before
committing it was the hack that made LOCK TABLE also work on FOREIGN
TABLEs. Since we're treating a foreign table as a different kind of
object than a TABLE in some places, we shouldn't confuse the two
things elsewhere, at least in my opinion. I also noticed that pg_dump
has the following comment:

* NOTE: it'd be kinda nice to lock other relations
too, not only
* plain tables, but the backend doesn't presently allow that.

This is pretty trivial from a backend point of view, especially with
the new objectaddress.c machinery. In a comparatively small amount of
code we could support locks not only on all types of relations but
also on any other backend objects we think it's worth supporting. The
only difficulty is the grammar: we allow either "LOCK relname" or
"LOCK TABLE relname", so adding e.g. "LOCK SEQUENCE relname" creates a
shift/reduce conflict because SEQUENCE is unreserved. We can easily
make "LOCK FOREIGN TABLE relname" work because FOREIGN is already
full-reserved, but that seems to be the only case that can be done
relatively painlessly.

So, the options as I see them are:

1. Do nothing.
2. Support LOCK FOREIGN TABLE relname and forget about the rest.
Feels fairly arbitrary, but avoids any hard decisions.
3. Partially reserve keywords like VIEW and SEQUENCE, and support LOCK
[ TABLE | VIEW | SEQUENCE | FOREIGN TABLE ] relname. Doesn't really
scale to other object types unless you keep reserving more keywords,
but maybe we don't care.
4. Make the keyword TABLE required, and support LOCK { TABLE | VIEW |
SEQUENCE | FOREIGN TABLE | maybe other object types } relname. This
is a backward-compatibility break, but so is reserving keywords, and
this approach has two redeeming virtues: (1) it only affects people
who are actually using "LOCK foo", whereas partially reserving
keywords will affect people using completely unrelated parts of the
system, and (2) it's pretty much future-proof - we can add more
relkinds or other object types down the road with no additional pain.
5. Create some alternative syntax for locking, and continue to support
the existing syntax for backward compatibility. We've done this
successfully with COPY (twice), EXPLAIN, and VACUUM, but it's not
clear how to do it here. You'd need either a different verb (and it's
not too clear what would make half as much sense as LOCK) or else a
syntax that involved putting something that can't be confused with a
table name immediately afterward. Something like LOCK (SEQUENCE foo)
would work, but that seems unspeakably ugly. LOCK IN [mode] { TABLE |
VIEW | SEQUENCE | FOREIGN TABLE | maybe other object types } relname
would work too, but that seems ugly and confusing also. Nothing else
is coming to mind at the moment.

On balance I think my vote is for #4. Other votes/ideas?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Jan 07, 2011 at 08:16:33AM -0500, Robert Haas wrote:
> One of the things that I ripped out of the SQL/MED syntax patch before
> committing it was the hack that made LOCK TABLE also work on FOREIGN
> TABLEs. Since we're treating a foreign table as a different kind of
> object than a TABLE in some places, we shouldn't confuse the two
> things elsewhere, at least in my opinion. I also noticed that pg_dump
> has the following comment:
>
> * NOTE: it'd be kinda nice to lock other relations
> too, not only
> * plain tables, but the backend doesn't presently allow that.
>
> This is pretty trivial from a backend point of view, especially with
> the new objectaddress.c machinery. In a comparatively small amount of
> code we could support locks not only on all types of relations but
> also on any other backend objects we think it's worth supporting. The
> only difficulty is the grammar: we allow either "LOCK relname" or
> "LOCK TABLE relname", so adding e.g. "LOCK SEQUENCE relname" creates a
> shift/reduce conflict because SEQUENCE is unreserved. We can easily
> make "LOCK FOREIGN TABLE relname" work because FOREIGN is already
> full-reserved, but that seems to be the only case that can be done
> relatively painlessly.
>
> So, the options as I see them are:
>
> 1. Do nothing.
> 2. Support LOCK FOREIGN TABLE relname and forget about the rest.
> Feels fairly arbitrary, but avoids any hard decisions.
> 3. Partially reserve keywords like VIEW and SEQUENCE, and support LOCK
> [ TABLE | VIEW | SEQUENCE | FOREIGN TABLE ] relname. Doesn't really
> scale to other object types unless you keep reserving more keywords,
> but maybe we don't care.
> 4. Make the keyword TABLE required, and support LOCK { TABLE | VIEW |
> SEQUENCE | FOREIGN TABLE | maybe other object types } relname.

I'm not sure I understand this. Does it mean I'd have to say

LOCK TABLE my_view;

? If so, I don't think that's a great idea. We used to have to do
TABLE operations on SEQUENCEs because they just happened to be
implemented as special tables, which wired implementation details into
the API. This is Generally Not A Good Thing™, and we removed that
some time back.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Fri, Jan 7, 2011 at 10:52 AM, David Fetter <david(at)fetter(dot)org> wrote:
> I'm not sure I understand this.  Does it mean I'd have to say
>
>    LOCK TABLE my_view;

No.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Jan 07, 2011 at 10:58:41AM -0500, Robert Haas wrote:
> On Fri, Jan 7, 2011 at 10:52 AM, David Fetter <david(at)fetter(dot)org> wrote:
> > I'm not sure I understand this.  Does it mean I'd have to say
> >
> >    LOCK TABLE my_view;
>
> No.

+1 for #4, then :)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Fri, 2011-01-07 at 08:16 -0500, Robert Haas wrote:

> One of the things that I ripped out of the SQL/MED syntax patch before
> committing it was the hack that made LOCK TABLE also work on FOREIGN
> TABLEs. Since we're treating a foreign table as a different kind of
> object than a TABLE in some places, we shouldn't confuse the two
> things elsewhere, at least in my opinion. I also noticed that pg_dump
> has the following comment:
>
> * NOTE: it'd be kinda nice to lock other relations
> too, not only
> * plain tables, but the backend doesn't presently
> allow that.
>
> This is pretty trivial from a backend point of view, especially with
> the new objectaddress.c machinery.

I'm not clear why we'd want to do that. We shouldn't just be adding
things because we can do them easily, we should be adding things with a
clear use case or a standardization requirement.

If anyone suggested tuning some aspect of the code, yet offered no
evidence that it was ever important, it would get shot down. Why is this
any different?

Allowing LOCK on views would significantly undermine admin structures
where the only access to a table is via a view. This would allow people
to lock objects they didn't previously have access to and seems likely
to introduce more contention into applications as a result.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Fri, Jan 7, 2011 at 12:17 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Fri, 2011-01-07 at 08:16 -0500, Robert Haas wrote:
>
>> One of the things that I ripped out of the SQL/MED syntax patch before
>> committing it was the hack that made LOCK TABLE also work on FOREIGN
>> TABLEs.  Since we're treating a foreign table as a different kind of
>> object than a TABLE in some places, we shouldn't confuse the two
>> things elsewhere, at least in my opinion.  I also noticed that pg_dump
>> has the following comment:
>>
>>               * NOTE: it'd be kinda nice to lock other relations
>> too, not only
>>                  * plain tables, but the backend doesn't presently
>> allow that.
>>
>> This is pretty trivial from a backend point of view, especially with
>> the new objectaddress.c machinery.
>
> I'm not clear why we'd want to do that. We shouldn't just be adding
> things because we can do them easily, we should be adding things with a
> clear use case or a standardization requirement.

Good point.

The reason why the pg_dump comment suggests that this feature would be
useful is that it would allow locking relations, and possibly other
objects, against concurrent drops. pg_dump can offer a
mostly-consistent view of the contents of every table in the system by
taking a snapshot at the beginning of operation and using that same
snapshot to completion. But that won't necessarily give it a
consistent view of the system catalogs, because those generally follow
SnapshotNow rules. To be fully consistent, it needs to take
AccessShareLocks on any schema objects it looks at. Currently, it can
do that for tables, but not other object types.

So suppose you pg_dump a view and and a function that uses the view.
In the middle of the dump, someone alters the view and the function in
a single transaction and commits it. You might dump the function
before the transaction commits and the view afterward, or visca versa,
and the result will be an inconsistent view of the database schema.
Allowing pg_dump to take AccessShareLocks on the objects in question
would prevent this sort of anomaly, which certainly seems to have some
value.

But if it doesn't have *enough* value, then we can go with the first
option I listed: do nothing.

> If anyone suggested tuning some aspect of the code, yet offered no
> evidence that it was ever important, it would get shot down. Why is this
> any different?

It's not.

> Allowing LOCK on views would significantly undermine admin structures
> where the only access to a table is via a view. This would allow people
> to lock objects they didn't previously have access to and seems likely
> to introduce more contention into applications as a result.

As long as we include appropriate privilege checks, this doesn't seem
like a major issue to me.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Jan7, 2011, at 22:21 , Robert Haas wrote:
> So suppose you pg_dump a view and and a function that uses the view.
> In the middle of the dump, someone alters the view and the function in
> a single transaction and commits it. You might dump the function
> before the transaction commits and the view afterward, or visca versa,
> and the result will be an inconsistent view of the database schema.
> Allowing pg_dump to take AccessShareLocks on the objects in question
> would prevent this sort of anomaly, which certainly seems to have some
> value.

That'd actually work fine I believe. AFAICT, system catalogs are *not*
accessed with SnapshotNow semantics if accessed via SQL, they're treated
like any other table in that case. The following confirms this

T1> BEGIN TRANSACTION ISOLATION SERIALIZABLE;
T1> SELECT TRUE; -- T1's snapshot is now set
T2> CREATE TABLE test(id int);
T1> SELECT * FROM test; -- Succeeds
T1> SELECT * FROM pg_class WHERE relname = 'test'; -- Returns 0 rows

Thus, all objects which are dumped purely by SQL-level inspection of the
system catalogs are safe I think. This is true for most objects I guess,
with the important exception being dumping a table's contents (but not
dumping its structure!). The lock makes sure that the structure we see
when inspecting the catalogs is also what "SELECT * FROM table" will return.
I dunno if there are any other objects like that, though - but if there
are, they could probably use a lock too.

Another class of failure cases can be constructed from output functions
which access the catalog. For example,

> CREATE TABLE my_types (a_type regtype);
> CREATE TYPE my_type AS (id int);
> INSERT INTO my_types VALUES ('my_type');

T1> BEGIN TRANSACTION ISOLATION SERIALIZABLE;
T1> SELECT TRUE; -- T1's snapshot is now set
T1> SELECT * FROM my_types;
a_type
---------
my_type

T2> BEGIN;
T2> DELETE FROM my_types WHERE a_type = 'my_type';
T2> DROP TYPE my_type;
T2> COMMIT;
T1> SELECT * FROM my_types;
a_type
--------
291919

best regards.
Florian Pflug

On Fri, Jan 7, 2011 at 5:17 PM, Florian Pflug <fgp(at)phlo(dot)org> wrote:
> On Jan7, 2011, at 22:21 , Robert Haas wrote:
>> So suppose you pg_dump a view and and a function that uses the view.
>> In the middle of the dump, someone alters the view and the function in
>> a single transaction and commits it.  You might dump the function
>> before the transaction commits and the view afterward, or visca versa,
>> and the result will be an inconsistent view of the database schema.
>> Allowing pg_dump to take AccessShareLocks on the objects in question
>> would prevent this sort of anomaly, which certainly seems to have some
>> value.
>
> That'd actually work fine I believe. AFAICT, system catalogs are *not*
> accessed with SnapshotNow semantics if accessed via SQL, they're treated
> like any other table in that case.

Oh, hm. Interesting point.

> Thus, all objects which are dumped purely by SQL-level inspection of the
> system catalogs are safe I think. This is true for most objects I guess,
> with the important exception being dumping a table's contents (but not
> dumping its structure!). The lock makes sure that the structure we see
> when inspecting the catalogs is also what "SELECT * FROM table" will return.
> I dunno if there are any other objects like that, though - but if there
> are, they could probably use a lock too.

Hmm. It would seem that to be vulnerable you'd need an object where
you need to dump both the structure and the contents, and I can't
think of any. Or it could apply to an object where you called some
pg_foo-ish function that did some SnapshotNow magic behind the scenes;
not sure if there are any of those, and this might not be the right
fix anyway.

> Another class of failure cases can be constructed from output functions
> which access the catalog. For example,
>
>> CREATE TABLE my_types (a_type regtype);
>> CREATE TYPE my_type AS (id int);
>> INSERT INTO my_types VALUES ('my_type');
>
> T1> BEGIN TRANSACTION ISOLATION SERIALIZABLE;
> T1> SELECT TRUE; -- T1's snapshot is now set
> T1> SELECT * FROM my_types;
>  a_type
> ---------
>  my_type
>
> T2> BEGIN;
> T2> DELETE FROM my_types WHERE a_type = 'my_type';
> T2> DROP TYPE my_type;
> T2> COMMIT;
> T1> SELECT * FROM my_types;
>  a_type
> --------
>  291919

Well, that happens even if there's no concurrency involved.

rhaas=# create table my_types (a_type regtype);
CREATE TABLE
rhaas=# create type my_type as (id int);
CREATE TYPE
rhaas=# insert into my_types values ('my_type');
INSERT 0 1
rhaas=# select * from my_types;
a_type
---------
my_type
(1 row)

rhaas=# drop type my_type;
DROP TYPE
rhaas=# select * from my_types;
a_type
--------
16474
(1 row)

If the conclusion of this discussion is that pg_dump doesn't really
need to lock anything other than tables, we should update the comments
to say that, rather than what they say now.

It's also worth thinking about whether there's any use case for
locking other types of objects for any reason *other than* pg_dump. I
can't immediately think of anything terribly compelling, but I might
be missing something.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Jan7, 2011, at 23:56 , Robert Haas wrote:
> On Fri, Jan 7, 2011 at 5:17 PM, Florian Pflug <fgp(at)phlo(dot)org> wrote:
>> Thus, all objects which are dumped purely by SQL-level inspection of the
>> system catalogs are safe I think. This is true for most objects I guess,
>> with the important exception being dumping a table's contents (but not
>> dumping its structure!). The lock makes sure that the structure we see
>> when inspecting the catalogs is also what "SELECT * FROM table" will return.
>> I dunno if there are any other objects like that, though - but if there
>> are, they could probably use a lock too.
>
> Hmm. It would seem that to be vulnerable you'd need an object where
> you need to dump both the structure and the contents, and I can't
> think of any. Or it could apply to an object where you called some
> pg_foo-ish function that did some SnapshotNow magic behind the scenes;
> not sure if there are any of those, and this might not be the right
> fix anyway.

I forgot about sequences earlier. If we dump while someone deletes all
rows and resets the sequence the dump might contain rows and still
reset the sequence. This could cause duplicate key errors on restore.
I haven't checked if this is really possible though - I guess it would
depend on the exact order of these events...

>> Another class of failure cases can be constructed from output functions
>> which access the catalog. For example,
>>
>>
>> <snipped>
>>
>> T2> BEGIN;
>> T2> DELETE FROM my_types WHERE a_type = 'my_type';
>> T2> DROP TYPE my_type;
>> T2> COMMIT;
>> T1> SELECT * FROM my_types;
>> a_type
>> --------
>> 291919
>
> Well, that happens even if there's no concurrency involved.

Not really, because the DROP TYPE and DELETE FROM my_types is
done within a transaction, so one might expect nobody else to
see the intermediate state. But yeah, I agree, this corner-case isn't
something we have to worry about too much.

> If the conclusion of this discussion is that pg_dump doesn't really
> need to lock anything other than tables, we should update the comments
> to say that, rather than what they say now.

+1

> It's also worth thinking about whether there's any use case for
> locking other types of objects for any reason *other than* pg_dump. I
> can't immediately think of anything terribly compelling, but I might
> be missing something.

I wonder how such locks would work. Would such locks prevent accessing
these objects? Or just modifications? For example, if I locked a function,
could someone else execute it while I held the lock?

best regards,
Florian Pflug

On Fri, Jan 7, 2011 at 6:28 PM, Florian Pflug <fgp(at)phlo(dot)org> wrote:
> I forgot about sequences earlier. If we dump while someone deletes all
> rows and resets the sequence the dump might contain rows and still
> reset the sequence. This could cause duplicate key errors on restore.
> I haven't checked if this is really possible though - I guess it would
> depend on the exact order of these events...

To fix this, you'd need a lock that allowed getting values from the
sequence but prevented resetting it, and...

> I wonder how such locks would work. Would such locks prevent accessing
> these objects? Or just modifications? For example, if I locked a function,
> could someone else execute it while I held the lock?

...in fact we do very little locking of objects other than tables.
DROP takes an AccessExclusiveLock on whatever it's dropping, and
COMMENT and SECURITY LABEL take ShareUpdateExclusiveLocks to avoid
orphaning pg_{sh,}description or pg_seclabel entries in the face of a
concurrent drop, but most operations on non-table objects don't AFAIK
take any lock at all. We probably don't want to make too many changes
in this area, because there are already workloads where the
heavyweight lock manager can become a bottleneck, and one can easily
imagine that locking operators or namespaces could make that problem
much worse.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> On Fri, Jan 7, 2011 at 6:28 PM, Florian Pflug <fgp(at)phlo(dot)org> wrote:
>> I forgot about sequences earlier. If we dump while someone deletes all
>> rows and resets the sequence the dump might contain rows and still
>> reset the sequence. This could cause duplicate key errors on restore.
>> I haven't checked if this is really possible though - I guess it would
>> depend on the exact order of these events...
>
> To fix this, you'd need a lock that allowed getting values from the
> sequence but prevented resetting it, and...
>
>> I wonder how such locks would work. Would such locks prevent accessing
>> these objects? Or just modifications? For example, if I locked a function,
>> could someone else execute it while I held the lock?
>
> ...in fact we do very little locking of objects other than tables.
> DROP takes an AccessExclusiveLock on whatever it's dropping, and
> COMMENT and SECURITY LABEL take ShareUpdateExclusiveLocks to avoid
> orphaning pg_{sh,}description or pg_seclabel entries in the face of a
> concurrent drop, but most operations on non-table objects don't AFAIK
> take any lock at all. We probably don't want to make too many changes
> in this area, because there are already workloads where the
> heavyweight lock manager can become a bottleneck, and one can easily
> imagine that locking operators or namespaces could make that problem
> much worse.

For query based replication tools like pgpool-II (I don't know any
other tools, for example Postgres XC falls in this category or
not...), we need to be able to lock sequences. Fortunately it is allowed to:

SELECT 1 FROM foo_sequece FOR UPDATE;

but LOCK foo_sequence looks more appropreate syntax for me.
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp

At present, XC does not seem to need locks to maintain cluster-wide
integrity because it is maintained centrally in GTM. If application
needs to do this, for example, to synchronize between different
clusters, XC needs this capability obviously.
----------
Koichi Suzuki

2011/1/11 Tatsuo Ishii <ishii(at)postgresql(dot)org>:
>> On Fri, Jan 7, 2011 at 6:28 PM, Florian Pflug <fgp(at)phlo(dot)org> wrote:
>>> I forgot about sequences earlier. If we dump while someone deletes all
>>> rows and resets the sequence the dump might contain rows and still
>>> reset the sequence. This could cause duplicate key errors on restore.
>>> I haven't checked if this is really possible though - I guess it would
>>> depend on the exact order of these events...
>>
>> To fix this, you'd need a lock that allowed getting values from the
>> sequence but prevented resetting it, and...
>>
>>> I wonder how such locks would work. Would such locks prevent accessing
>>> these objects? Or just modifications? For example, if I locked a function,
>>> could someone else execute it while I held the lock?
>>
>> ...in fact we do very little locking of objects other than tables.
>> DROP takes an AccessExclusiveLock on whatever it's dropping, and
>> COMMENT and SECURITY LABEL take ShareUpdateExclusiveLocks to avoid
>> orphaning pg_{sh,}description or pg_seclabel entries in the face of a
>> concurrent drop, but most operations on non-table objects don't AFAIK
>> take any lock at all.  We probably don't want to make too many changes
>> in this area, because there are already workloads where the
>> heavyweight lock manager can become a bottleneck, and one can easily
>> imagine that locking operators or namespaces could make that problem
>> much worse.
>
> For query based replication tools like pgpool-II (I don't know any
> other tools, for example Postgres XC falls in this category or
> not...), we need to be able to lock sequences. Fortunately it is allowed to:
>
> SELECT 1 FROM foo_sequece FOR UPDATE;
>
> but LOCK foo_sequence looks more appropreate syntax for me.
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese: http://www.sraoss.co.jp
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

On Tue, Jan 11, 2011 at 4:46 AM, Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
>> On Fri, Jan 7, 2011 at 6:28 PM, Florian Pflug <fgp(at)phlo(dot)org> wrote:
>>> I forgot about sequences earlier. If we dump while someone deletes all
>>> rows and resets the sequence the dump might contain rows and still
>>> reset the sequence. This could cause duplicate key errors on restore.
>>> I haven't checked if this is really possible though - I guess it would
>>> depend on the exact order of these events...
>>
>> To fix this, you'd need a lock that allowed getting values from the
>> sequence but prevented resetting it, and...
>>
>>> I wonder how such locks would work. Would such locks prevent accessing
>>> these objects? Or just modifications? For example, if I locked a function,
>>> could someone else execute it while I held the lock?
>>
>> ...in fact we do very little locking of objects other than tables.
>> DROP takes an AccessExclusiveLock on whatever it's dropping, and
>> COMMENT and SECURITY LABEL take ShareUpdateExclusiveLocks to avoid
>> orphaning pg_{sh,}description or pg_seclabel entries in the face of a
>> concurrent drop, but most operations on non-table objects don't AFAIK
>> take any lock at all.  We probably don't want to make too many changes
>> in this area, because there are already workloads where the
>> heavyweight lock manager can become a bottleneck, and one can easily
>> imagine that locking operators or namespaces could make that problem
>> much worse.
>
> For query based replication tools like pgpool-II (I don't know any
> other tools, for example Postgres XC falls in this category or
> not...), we need to be able to lock sequences. Fortunately it is allowed to:
>
> SELECT 1 FROM foo_sequece FOR UPDATE;
>
> but LOCK foo_sequence looks more appropreate syntax for me.

Those aren't doing the same thing. The first is locking the one and
only tuple that is contained within the sequence, while the second is
locking the sequence object itself.

At this point, I'm inclined to think that the pg_dump comment is just
wrong, and we ought to fix it to say that we don't really want to be
able to lock other relations after all, and call it good.

As a side node, locking a sequence for replication seems like it could
have pretty dire effects on performance in certain workloads. Why do
you need to do that, anyway?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> On Tue, Jan 11, 2011 at 4:46 AM, Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
>>> On Fri, Jan 7, 2011 at 6:28 PM, Florian Pflug <fgp(at)phlo(dot)org> wrote:
>>>> I forgot about sequences earlier. If we dump while someone deletes all
>>>> rows and resets the sequence the dump might contain rows and still
>>>> reset the sequence. This could cause duplicate key errors on restore.
>>>> I haven't checked if this is really possible though - I guess it would
>>>> depend on the exact order of these events...
>>>
>>> To fix this, you'd need a lock that allowed getting values from the
>>> sequence but prevented resetting it, and...
>>>
>>>> I wonder how such locks would work. Would such locks prevent accessing
>>>> these objects? Or just modifications? For example, if I locked a function,
>>>> could someone else execute it while I held the lock?
>>>
>>> ...in fact we do very little locking of objects other than tables.
>>> DROP takes an AccessExclusiveLock on whatever it's dropping, and
>>> COMMENT and SECURITY LABEL take ShareUpdateExclusiveLocks to avoid
>>> orphaning pg_{sh,}description or pg_seclabel entries in the face of a
>>> concurrent drop, but most operations on non-table objects don't AFAIK
>>> take any lock at all.  We probably don't want to make too many changes
>>> in this area, because there are already workloads where the
>>> heavyweight lock manager can become a bottleneck, and one can easily
>>> imagine that locking operators or namespaces could make that problem
>>> much worse.
>>
>> For query based replication tools like pgpool-II (I don't know any
>> other tools, for example Postgres XC falls in this category or
>> not...), we need to be able to lock sequences. Fortunately it is allowed to:
>>
>> SELECT 1 FROM foo_sequece FOR UPDATE;
>>
>> but LOCK foo_sequence looks more appropreate syntax for me.
>
> Those aren't doing the same thing. The first is locking the one and
> only tuple that is contained within the sequence, while the second is
> locking the sequence object itself.

But a sequence relation contains only 1 tuple and there's no
difference among them, no?

> At this point, I'm inclined to think that the pg_dump comment is just
> wrong, and we ought to fix it to say that we don't really want to be
> able to lock other relations after all, and call it good.
>
> As a side node, locking a sequence for replication seems like it could
> have pretty dire effects on performance in certain workloads. Why do
> you need to do that, anyway?

Pgpool not only needs to replicate sequences but replicates tuples
updated by DMLs which are using sequence value(I am talking about
SERIAL data types). For this purpose, pgpool issue nextval() to master
DB server first, then use the value for subsequent INSERT/UPDATE. This
will guarantee that inserted/updated values using sequences are
identical among master and slave DB servers. Problem is, if this
process happens in concurrent sessions, inserted/updated tuples might
not have identical value among DB servers. So I need "sequence lock"
here. This is the price statement based replication tools have to pay
for:-<
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp

On Tue, Jan 11, 2011 at 6:31 AM, Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
>>> For query based replication tools like pgpool-II (I don't know any
>>> other tools, for example Postgres XC falls in this category or
>>> not...), we need to be able to lock sequences. Fortunately it is allowed to:
>>>
>>> SELECT 1 FROM foo_sequece FOR UPDATE;
>>>
>>> but LOCK foo_sequence looks more appropreate syntax for me.
>>
>> Those aren't doing the same thing.  The first is locking the one and
>> only tuple that is contained within the sequence, while the second is
>> locking the sequence object itself.
>
> But a sequence relation contains only 1 tuple and there's no
> difference among them, no?

No, not really. It's still a different object.

>> As a side node, locking a sequence for replication seems like it could
>> have pretty dire effects on performance in certain workloads.  Why do
>> you need to do that, anyway?
>
> Pgpool not only needs to replicate sequences but replicates tuples
> updated by DMLs which are using sequence value(I am talking about
> SERIAL data types). For this purpose, pgpool issue nextval() to master
> DB server first, then use the value for subsequent INSERT/UPDATE. This
> will guarantee that inserted/updated values using sequences are
> identical among master and slave DB servers. Problem is, if this
> process happens in concurrent sessions, inserted/updated tuples might
> not have identical value among DB servers. So I need "sequence lock"
> here. This is the price statement based replication tools have to pay
> for:-<

Ouch.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Jan 11, 2011 at 4:46 AM, Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
>> For query based replication tools like pgpool-II (I don't know any
>> other tools, for example Postgres XC falls in this category or
>> not...), we need to be able to lock sequences. Fortunately it is allowed to:
>>
>> SELECT 1 FROM foo_sequece FOR UPDATE;
>>
>> but LOCK foo_sequence looks more appropreate syntax for me.

> Those aren't doing the same thing. The first is locking the one and
> only tuple that is contained within the sequence, while the second is
> locking the sequence object itself.

> At this point, I'm inclined to think that the pg_dump comment is just
> wrong, and we ought to fix it to say that we don't really want to be
> able to lock other relations after all, and call it good.

The reason that pg_dump tries to acquire locks at all is to ensure that
it dumps a consistent view of the database. The general excuse for not
locking non-table objects is that (at least in most cases) they are
defined by single catalog entries and so there's no way to see a
non-self-consistent view of them. Tables, being defined by a collection
of rows in different catalogs, are *very* risky to dump without any
lock. This doesn't get noticeably better for non-table relation types.

An example of the sort of risk I'm thinking about is dumping a view
without any lock while someone else does a CREATE OR REPLACE VIEW on
it. You could very easily see a set of attributes (in pg_attribute)
that don't agree with the view rules you pulled from pg_rewrite. The
risk is minimal now since we don't allow C.O.R.V. to change the column
set, but as soon as somebody creates a patch that allows that, pg_dump
will have a problem.

Note that using a serializable transaction (with or without "true"
serializability) doesn't fix this issue, since pg_dump depends so
heavily on backend-side support functions that work in SnapshotNow mode.
It really needs locks to ensure that the support functions see a view
consistent with its own catalog reads.

In the SEQUENCE example above, SELECT ... FOR UPDATE is certainly not
adequate to protect the sequence against DDL-level changes. Fortunately
sequences don't have too many DDL commands, but still an ALTER RENAME
might be enough to confuse pg_dump.

(By the way, does that SELECT ... FOR UPDATE actually accomplish
anything at all? nextval() doesn't go through heap_update, and neither
does ALTER SEQUENCE, so I'd be a bit surprised if it really manages to
block changes to the sequence.)

regards, tom lane

> In the SEQUENCE example above, SELECT ... FOR UPDATE is certainly not
> adequate to protect the sequence against DDL-level changes. Fortunately
> sequences don't have too many DDL commands, but still an ALTER RENAME
> might be enough to confuse pg_dump.
>
> (By the way, does that SELECT ... FOR UPDATE actually accomplish
> anything at all? nextval() doesn't go through heap_update, and neither
> does ALTER SEQUENCE, so I'd be a bit surprised if it really manages to
> block changes to the sequence.)

Of course "SELECT ... FOR UPDATE" does not block nextval(). It just
blocks concurrent "SELECT ... FOR UPDATE" in other session. This is
enough for pgpool's use case.
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp

On Tue, Jan 11, 2011 at 10:35 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Tue, Jan 11, 2011 at 4:46 AM, Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
>>> For query based replication tools like pgpool-II (I don't know any
>>> other tools, for example Postgres XC falls in this category or
>>> not...), we need to be able to lock sequences. Fortunately it is allowed to:
>>>
>>> SELECT 1 FROM foo_sequece FOR UPDATE;
>>>
>>> but LOCK foo_sequence looks more appropreate syntax for me.
>
>> Those aren't doing the same thing.  The first is locking the one and
>> only tuple that is contained within the sequence, while the second is
>> locking the sequence object itself.
>
>> At this point, I'm inclined to think that the pg_dump comment is just
>> wrong, and we ought to fix it to say that we don't really want to be
>> able to lock other relations after all, and call it good.
>
> The reason that pg_dump tries to acquire locks at all is to ensure that
> it dumps a consistent view of the database.  The general excuse for not
> locking non-table objects is that (at least in most cases) they are
> defined by single catalog entries and so there's no way to see a
> non-self-consistent view of them.  Tables, being defined by a collection
> of rows in different catalogs, are *very* risky to dump without any
> lock.  This doesn't get noticeably better for non-table relation types.
>
> An example of the sort of risk I'm thinking about is dumping a view
> without any lock while someone else does a CREATE OR REPLACE VIEW on
> it.  You could very easily see a set of attributes (in pg_attribute)
> that don't agree with the view rules you pulled from pg_rewrite.  The
> risk is minimal now since we don't allow C.O.R.V. to change the column
> set, but as soon as somebody creates a patch that allows that, pg_dump
> will have a problem.

Actually, we do allow C.O.R.V. to do just that - I believe since 8.4.

rhaas=# create view v(a) as select 1;
CREATE VIEW
rhaas=# create or replace view v(a,b) as select 1, 2;
CREATE VIEW

> Note that using a serializable transaction (with or without "true"
> serializability) doesn't fix this issue, since pg_dump depends so
> heavily on backend-side support functions that work in SnapshotNow mode.
> It really needs locks to ensure that the support functions see a view
> consistent with its own catalog reads.

In that case, can I have some comments on approaches mentioned in my OP?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jan 11, 2011 at 8:31 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> In that case, can I have some comments on approaches mentioned in my OP?

Tom - I am willing to implement this if you think it's valuable, but
I'd like your input on the syntax.

http://archives.postgresql.org/pgsql-hackers/2011-01/msg00472.php

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Tom - I am willing to implement this if you think it's valuable, but
> I'd like your input on the syntax.
> http://archives.postgresql.org/pgsql-hackers/2011-01/msg00472.php

It looks to me like the reason why there's a shift/reduce conflict is
not so much that TABLE is optional as that we allow the syntax

LOCK tablename NOWAIT

If that weren't possible, then a table name would have to be followed by
EOL or IN (which is full-reserved), while an optional object type name
could not be followed by either, so there would be no shift/reduce
conflict. So we broke it when we added NOWAIT, not when TABLE was made
optional.

So it looks to me like there are at least two fixes other than the ones
you enumerated:

1. Make NOWAIT a reserved word. Not good, but perhaps better than
reserving all the different object type names.

2. Disallow the above abbreviated syntax; allow NOWAIT only after an
explicit IN ... MODE phrase. This would probably break a couple of
applications, but I bet a lot fewer than changing the longer-established
parts of the command syntax would break.

I think #2 might be the best choice here.

regards, tom lane

On Fri, 2011-01-14 at 13:58 -0500, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > Tom - I am willing to implement this if you think it's valuable, but
> > I'd like your input on the syntax.
> > http://archives.postgresql.org/pgsql-hackers/2011-01/msg00472.php
>
> It looks to me like the reason why there's a shift/reduce conflict is
> not so much that TABLE is optional as that we allow the syntax
>
> LOCK tablename NOWAIT
>
> If that weren't possible, then a table name would have to be followed by
> EOL or IN (which is full-reserved), while an optional object type name
> could not be followed by either, so there would be no shift/reduce
> conflict. So we broke it when we added NOWAIT, not when TABLE was made
> optional.
>
> So it looks to me like there are at least two fixes other than the ones
> you enumerated:
>
> 1. Make NOWAIT a reserved word. Not good, but perhaps better than
> reserving all the different object type names.
>
> 2. Disallow the above abbreviated syntax; allow NOWAIT only after an
> explicit IN ... MODE phrase. This would probably break a couple of
> applications, but I bet a lot fewer than changing the longer-established
> parts of the command syntax would break.
>
> I think #2 might be the best choice here.

There's a similar issue on my new syntax for skipping the validation
check on FKs. I'd appreciate it if you could propose a solution there
also. I'm not sure whether I solved it, or am adding issues for the
future.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Fri, Jan 14, 2011 at 1:58 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Tom - I am willing to implement this if you think it's valuable, but
>> I'd like your input on the syntax.
>> http://archives.postgresql.org/pgsql-hackers/2011-01/msg00472.php
>
> It looks to me like the reason why there's a shift/reduce conflict is
> not so much that TABLE is optional as that we allow the syntax
>
>        LOCK tablename NOWAIT
>
> If that weren't possible, then a table name would have to be followed by
> EOL or IN (which is full-reserved), while an optional object type name
> could not be followed by either, so there would be no shift/reduce
> conflict.  So we broke it when we added NOWAIT, not when TABLE was made
> optional.

Hmm, OK.

> So it looks to me like there are at least two fixes other than the ones
> you enumerated:
>
> 1. Make NOWAIT a reserved word.  Not good, but perhaps better than
> reserving all the different object type names.
>
> 2. Disallow the above abbreviated syntax; allow NOWAIT only after an
> explicit IN ... MODE phrase.  This would probably break a couple of
> applications, but I bet a lot fewer than changing the longer-established
> parts of the command syntax would break.
>
> I think #2 might be the best choice here.

That strikes me as pretty unintuitive. I'd rather take the hit of
forcing people to write "LOCK TABLE foo" instead of just "LOCK foo"
than try to explain why they have to include "IN ACCESS EXCLUSIVE
MODE" if they want to stick "NOWAIT" on the end. However, I guess
it's a matter of opinion so... anyone else have an opinion?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Jan 14, 2011 at 1:58 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> 2. Disallow the above abbreviated syntax; allow NOWAIT only after an
>> explicit IN ... MODE phrase. This would probably break a couple of
>> applications, but I bet a lot fewer than changing the longer-established
>> parts of the command syntax would break.

> That strikes me as pretty unintuitive. I'd rather take the hit of
> forcing people to write "LOCK TABLE foo" instead of just "LOCK foo"
> than try to explain why they have to include "IN ACCESS EXCLUSIVE
> MODE" if they want to stick "NOWAIT" on the end. However, I guess
> it's a matter of opinion so... anyone else have an opinion?

It doesn't seem amazingly unintuitive to me; the syntax diagram just
changes from

LOCK ... [ IN lockmode MODE ] [ NOWAIT ]

to

LOCK ... [ IN lockmode MODE [ NOWAIT ] ]

If it had been that way to start with, nobody would have questioned it.

In any case I'd rather break apps using "LOCK foo NOWAIT" than break
every application using any form of LOCK at all, which is what I think
your proposal will amount to in practice. People aren't that eager to
write useless noise words, which is what TABLE has been up to now in
this command.

regards, tom lane

Le 14 janv. 2011 à 20:08, Robert Haas <robertmhaas(at)gmail(dot)com> a écrit :

> On Fri, Jan 14, 2011 at 1:58 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
>> So it looks to me like there are at least two fixes other than the ones
>> you enumerated:
>>
>> 1. Make NOWAIT a reserved word. Not good, but perhaps better than
>> reserving all the different object type names.
>>
>> 2. Disallow the above abbreviated syntax; allow NOWAIT only after an
>> explicit IN ... MODE phrase. This would probably break a couple of
>> applications, but I bet a lot fewer than changing the longer-established
>> parts of the command syntax would break.
>>
>> I think #2 might be the best choice here.

+1

>
> That strikes me as pretty unintuitive. I'd rather take the hit of
> forcing people to write "LOCK TABLE foo" instead of just "LOCK foo"
> than try to explain why they have to include "IN ACCESS EXCLUSIVE
> MODE" if they want to stick "NOWAIT" on the end. However, I guess
> it's a matter of opinion so... anyone else have an opinion?

Since you ask, see above. :)

--
dim

On Fri, 2011-01-14 at 14:48 -0500, Tom Lane wrote:

> In any case I'd rather break apps using "LOCK foo NOWAIT" than break
> every application using any form of LOCK at all, which is what I think
> your proposal will amount to in practice.

Can I suggest that we don't break anything at all?

pg_lock_object(objectname, objecttype, mode);
or
pg_lock_sequence(name, mode);

is all we need...

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On Fri, 2011-01-14 at 14:48 -0500, Tom Lane wrote:
>> In any case I'd rather break apps using "LOCK foo NOWAIT" than break
>> every application using any form of LOCK at all, which is what I think
>> your proposal will amount to in practice.

> Can I suggest that we don't break anything at all?

> pg_lock_object(objectname, objecttype, mode);
> or
> pg_lock_sequence(name, mode);

> is all we need...

No, that will not work at all. LOCK has to be a utility command.
A function called by SELECT isn't a substitute, because SELECT
will acquire a transaction snapshot before executing the function,
and that breaks many use cases for locks.

regards, tom lane

On Fri, 2011-01-14 at 15:05 -0500, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > On Fri, 2011-01-14 at 14:48 -0500, Tom Lane wrote:
> >> In any case I'd rather break apps using "LOCK foo NOWAIT" than break
> >> every application using any form of LOCK at all, which is what I think
> >> your proposal will amount to in practice.
>
> > Can I suggest that we don't break anything at all?
>
> > pg_lock_object(objectname, objecttype, mode);
> > or
> > pg_lock_sequence(name, mode);
>
> > is all we need...
>
> No, that will not work at all. LOCK has to be a utility command.
> A function called by SELECT isn't a substitute, because SELECT
> will acquire a transaction snapshot before executing the function,
> and that breaks many use cases for locks.

But it doesn't break the use case for locking sequences, ISTM.

Anyway, any suggestion that randomly breaks user applications is not
good. If there is a good reason to do that, OK, but I don't see that
here.

It's a major undertaking trying to write software that runs against
PostgreSQL for more than one release. We should be making that easier,
not harder.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On Fri, 2011-01-14 at 15:05 -0500, Tom Lane wrote:
>> No, that will not work at all. LOCK has to be a utility command.

> But it doesn't break the use case for locking sequences, ISTM.

You haven't stated what you think that use case is, but in any case
I'm sure someone can come up with another one where not freezing
the transaction snapshot *is* a consideration.

> Anyway, any suggestion that randomly breaks user applications is not
> good. If there is a good reason to do that, OK, but I don't see that
> here.

The good reason is adding functionality. Or is it your position that
the functionality under discussion is not worth any syntax breakage,
no matter how narrowly circumscribed? If we take that position then
we can drop this whole thread, because nothing's going to happen.

regards, tom lane

On Fri, 2011-01-14 at 15:46 -0500, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > On Fri, 2011-01-14 at 15:05 -0500, Tom Lane wrote:
> >> No, that will not work at all. LOCK has to be a utility command.
>
> > But it doesn't break the use case for locking sequences, ISTM.
>
> You haven't stated what you think that use case is, but in any case
> I'm sure someone can come up with another one where not freezing
> the transaction snapshot *is* a consideration.

> > Anyway, any suggestion that randomly breaks user applications is not
> > good. If there is a good reason to do that, OK, but I don't see that
> > here.
>
> The good reason is adding functionality. Or is it your position that
> the functionality under discussion is not worth any syntax breakage,
> no matter how narrowly circumscribed?

I'm willing to listen to a clear restatement of the functionality being
added, so we can compare that to what we will lose. Currently, IMHO, the
importance of the new functionality seems low and the effect of breakage
is high for our users.

And I'm also interested in exploring other ways that give us the
functionality requested without the breakage.

Evolution, not revolution.

> If we take that position then
> we can drop this whole thread, because nothing's going to happen.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

I wrote:
> It looks to me like the reason why there's a shift/reduce conflict is
> not so much that TABLE is optional as that we allow the syntax
> LOCK tablename NOWAIT

BTW, I did confirm this to the extent of showing that the shift-reduce
conflict could be eliminated by attaching precedences to SEQUENCE and
NOWAIT, a la

%nonassoc NOWAIT
%nonassoc SEQUENCE

This causes the ambiguous case

LOCK SEQUENCE NOWAIT

to be resolved by reducing SEQUENCE to unreserved_keyword, ie it's a
NOWAIT request for a table named "sequence", which is backwards
compatible. However, I'm not seriously proposing this as a usable fix.
I think there's far too much risk of unforeseen side-effects on the
behavor of other productions. We'd have to similarly attach a
precedence to every object-type keyword that we cared to use in LOCK,
and that would mean the potential for bollixing the behavior of an awful
lot of cases.

I think the realistic options are (1) change the syntax
non-backward-compatibly or (2) don't add any functionality here.

regards, tom lane

On Fri, 2011-01-14 at 16:09 -0500, Tom Lane wrote:

> I think the realistic options are (1) change the syntax
> non-backward-compatibly or (2) don't add any functionality here.

(3) think of another way.

I'm not keen to explain to people how we broke their applications just
because we wanted to add new functionality AND avoid one shift/reduce
conflict in our SQL grammar. Avoiding changes to user code isn't third
on that list of three things I want, its first.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> It's a major undertaking trying to write software that runs against
> PostgreSQL for more than one release. We should be making that easier,
> not harder.

None of the proposals would make it impossible to write a LOCK statement
that works on all available releases, so I think the above argument is
a tad off-topic.

regards, tom lane

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On Fri, 2011-01-14 at 16:09 -0500, Tom Lane wrote:
>> I think the realistic options are (1) change the syntax
>> non-backward-compatibly or (2) don't add any functionality here.

> (3) think of another way.

The only third way that I can see is to invent some new, unrelated
syntax for the new facilities. For example (just a straw man)

ACQUIRE LOCK FOR object-type object-name opt-lock-type opt-no-wait

The objection to this approach (which can also be lodged against your
function proposal) is that it's a larger burden on users: now they have
two syntaxes to remember, more complicated code to write if it needs to
use both syntaxes, etc. In the long run this is more trouble than a
one-time adjustment, especially if that adjustment can be limited to a
small number of uncommon cases.

> I'm not keen to explain to people how we broke their applications just
> because we wanted to add new functionality AND avoid one shift/reduce
> conflict in our SQL grammar. Avoiding changes to user code isn't third
> on that list of three things I want, its first.

I grow weary of discussions in which somebody argues that consideration
X always outweighs every other consideration. We're doing engineering
here, not theology, and there are always tradeoffs to be made. In this
case it's my opinion that a small syntax adjustment is the best
tradeoff.

regards, tom lane

On Fri, Jan 14, 2011 at 5:34 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I'm not keen to explain to people how we broke their applications just
>> because we wanted to add new functionality AND avoid one shift/reduce
>> conflict in our SQL grammar. Avoiding changes to user code isn't third
>> on that list of three things I want, its first.
>
> I grow weary of discussions in which somebody argues that consideration
> X always outweighs every other consideration.  We're doing engineering
> here, not theology, and there are always tradeoffs to be made.  In this
> case it's my opinion that a small syntax adjustment is the best
> tradeoff.

Me, too. But I don't agree with your particular choice of small
syntax adjustment. Maybe we should just let the issue drop for now.
Nobody's actually complained about this that I can recall; it's just a
comment that's been sitting there in pg_dump for ages, and I was
inspired to think of it again because of the SQL/MED work. I'm not
sufficiently in love with this idea to walk through fire for it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Me, too. But I don't agree with your particular choice of small
> syntax adjustment. Maybe we should just let the issue drop for now.
> Nobody's actually complained about this that I can recall; it's just a
> comment that's been sitting there in pg_dump for ages, and I was
> inspired to think of it again because of the SQL/MED work. I'm not
> sufficiently in love with this idea to walk through fire for it.

Agreed. Once there's some pressing need for it, it'll be easier to make
the case that some amount of incompatibility is acceptable.

regards, tom lane

On Fri, 2011-01-14 at 17:34 -0500, Tom Lane wrote:
>
> > I'm not keen to explain to people how we broke their applications
> just
> > because we wanted to add new functionality AND avoid one
> shift/reduce
> > conflict in our SQL grammar. Avoiding changes to user code isn't
> third
> > on that list of three things I want, its first.
>
> I grow weary of discussions in which somebody argues that
> consideration X always outweighs every other consideration. We're
> doing engineering here, not theology, and there are always tradeoffs
> to be made. In this case it's my opinion that a small syntax
> adjustment is the best tradeoff.

I didn't say avoiding changes to user code *always* outweighs other
considerations, it just does so in this case, for me. I too am doing
engineering, not theology; our opinions differ in one area, that's all.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Jan15, 2011, at 02:03 , Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Me, too. But I don't agree with your particular choice of small
>> syntax adjustment. Maybe we should just let the issue drop for now.
>> Nobody's actually complained about this that I can recall; it's just a
>> comment that's been sitting there in pg_dump for ages, and I was
>> inspired to think of it again because of the SQL/MED work. I'm not
>> sufficiently in love with this idea to walk through fire for it.
>
> Agreed. Once there's some pressing need for it, it'll be easier to make
> the case that some amount of incompatibility is acceptable.

Assuming that day will come eventually, should we deprecate the LOCK <table>
shortcut now to ease the transition later? If people want that, I could go
through the docs and add some appropriate warnings.

best regards,
Florian Pflug

On Sat, 2011-01-15 at 12:19 +0100, Florian Pflug wrote:
> On Jan15, 2011, at 02:03 , Tom Lane wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> >> Me, too. But I don't agree with your particular choice of small
> >> syntax adjustment. Maybe we should just let the issue drop for now.
> >> Nobody's actually complained about this that I can recall; it's just a
> >> comment that's been sitting there in pg_dump for ages, and I was
> >> inspired to think of it again because of the SQL/MED work. I'm not
> >> sufficiently in love with this idea to walk through fire for it.
> >
> > Agreed. Once there's some pressing need for it, it'll be easier to make
> > the case that some amount of incompatibility is acceptable.
>
> Assuming that day will come eventually, should we deprecate the LOCK <table>
> shortcut now to ease the transition later? If people want that, I could go
> through the docs and add some appropriate warnings.

Sounds good to me.

I think we should have a section in the release notes on Deprecated
Features, noting that certain things will be removed later and should be
changed now and not relied upon in the future. A pending
incompatibilities list.

I would urge people to come up with a much wider list of "things we
don't like" so we can more easily avoid discussions like this in the
future. Forward planning helps make change easier.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Jan 15, 2011 12:30 PM, "Simon Riggs" <simon(at)2ndquadrant(dot)com> wrote:
>
> On Sat, 2011-01-15 at 12:19 +0100, Florian Pflug wrote:
> > On Jan15, 2011, at 02:03 , Tom Lane wrote:
> > > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > >> Me, too. But I don't agree with your particular choice of small
> > >> syntax adjustment. Maybe we should just let the issue drop for now.
> > >> Nobody's actually complained about this that I can recall; it's just
a
> > >> comment that's been sitting there in pg_dump for ages, and I was
> > >> inspired to think of it again because of the SQL/MED work. I'm not
> > >> sufficiently in love with this idea to walk through fire for it.
> > >
> > > Agreed. Once there's some pressing need for it, it'll be easier to
make
> > > the case that some amount of incompatibility is acceptable.
> >
> > Assuming that day will come eventually, should we deprecate the LOCK
<table>
> > shortcut now to ease the transition later? If people want that, I could
go
> > through the docs and add some appropriate warnings.
>
> Sounds good to me.
>
>
> I think we should have a section in the release notes on Deprecated
> Features, noting that certain things will be removed later and should be
> changed now and not relied upon in the future. A pending
> incompatibilities list.

+1. This would be very useful. Its hard enough for us "on the inside" to
keep track of things that we deprecated...

> I would urge people to come up with a much wider list of "things we
> don't like" so we can more easily avoid discussions like this in the
> future. Forward planning helps make change easier.

There is a section on the TODO for that already, i think. Seems reasonable
since this is more for developers than users.

/Magnus

On Sat, Jan 15, 2011 at 6:29 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Sat, 2011-01-15 at 12:19 +0100, Florian Pflug wrote:
>> On Jan15, 2011, at 02:03 , Tom Lane wrote:
>> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> >> Me, too.  But I don't agree with your particular choice of small
>> >> syntax adjustment.  Maybe we should just let the issue drop for now.
>> >> Nobody's actually complained about this that I can recall; it's just a
>> >> comment that's been sitting there in pg_dump for ages, and I was
>> >> inspired to think of it again because of the SQL/MED work.  I'm not
>> >> sufficiently in love with this idea to walk through fire for it.
>> >
>> > Agreed.  Once there's some pressing need for it, it'll be easier to make
>> > the case that some amount of incompatibility is acceptable.
>>
>> Assuming that day will come eventually, should we deprecate the LOCK <table>
>> shortcut now to ease the transition later? If people want that, I could go
>> through the docs and add some appropriate warnings.
>
> Sounds good to me.

+1.

> I think we should have a section in the release notes on Deprecated
> Features, noting that certain things will be removed later and should be
> changed now and not relied upon in the future. A pending
> incompatibilities list.

Agreed. Of course, the problem is sometimes we don't do what we say
we're going to do, but it's worth a try.

> I would urge people to come up with a much wider list of "things we
> don't like" so we can more easily avoid discussions like this in the
> future. Forward planning helps make change easier.

Optional keywords!

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sat, Jan 15, 2011 at 6:29 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> I think we should have a section in the release notes on Deprecated
>> Features, noting that certain things will be removed later and should be
>> changed now and not relied upon in the future. A pending
>> incompatibilities list.

> Agreed. Of course, the problem is sometimes we don't do what we say
> we're going to do, but it's worth a try.

I think if we had a formal list of planned removals, it'd be more likely
that they'd actually happen. Right now there's no process at all
driving such things forward.

I suggest also marking each item with a release in which we intend to do
it, so we don't have to try to remember whether a reasonable amount of
time has elapsed.

regards, tom lane

On Sat, Jan 15, 2011 at 10:25 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Sat, Jan 15, 2011 at 6:29 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>> I think we should have a section in the release notes on Deprecated
>>> Features, noting that certain things will be removed later and should be
>>> changed now and not relied upon in the future. A pending
>>> incompatibilities list.
>
>> Agreed.  Of course, the problem is sometimes we don't do what we say
>> we're going to do, but it's worth a try.
>
> I think if we had a formal list of planned removals, it'd be more likely
> that they'd actually happen.  Right now there's no process at all
> driving such things forward.

OK.

> I suggest also marking each item with a release in which we intend to do
> it, so we don't have to try to remember whether a reasonable amount of
> time has elapsed.

You mean like the way the 9.1devel documentation says that
contrib/xml2 will be removed in 8.4? I wonder if we'll do anything
either about deprecating the module or about changing the
documentation before 8.4 is EOL.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sat, Jan 15, 2011 at 10:25 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I suggest also marking each item with a release in which we intend to do
>> it, so we don't have to try to remember whether a reasonable amount of
>> time has elapsed.

> You mean like the way the 9.1devel documentation says that
> contrib/xml2 will be removed in 8.4? I wonder if we'll do anything
> either about deprecating the module or about changing the
> documentation before 8.4 is EOL.

Well, that one is a special case, because we knew perfectly well that we
hadn't replaced all the functionality of xml2 (and we still haven't).
I think the "official" deprecation list should only contain items for
which there's no blocking issue other than wanting to give users time to
migrate to an existing alternate solution.

regards, tom lane

On Sat, Jan 15, 2011 at 3:37 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Sat, Jan 15, 2011 at 10:25 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> I suggest also marking each item with a release in which we intend to do
>>> it, so we don't have to try to remember whether a reasonable amount of
>>> time has elapsed.
>
>> You mean like the way the 9.1devel documentation says that
>> contrib/xml2 will be removed in 8.4?  I wonder if we'll do anything
>> either about deprecating the module or about changing the
>> documentation before 8.4 is EOL.
>
> Well, that one is a special case, because we knew perfectly well that we
> hadn't replaced all the functionality of xml2 (and we still haven't).
> I think the "official" deprecation list should only contain items for
> which there's no blocking issue other than wanting to give users time to
> migrate to an existing alternate solution.

Fair enough.

Do we wish to officially document LOCK without TABLE as a good idea to
start avoiding, in case we decide to do something about that in the
future?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Do we wish to officially document LOCK without TABLE as a good idea to
> start avoiding, in case we decide to do something about that in the
> future?

I'm still not for fixing the ambiguity that way. TABLE is an optional
noise word in other contexts, notably GRANT/REVOKE where that syntax is
dictated by SQL standard. It would be inconsistent to have it be
required in LOCK.

I think we should deprecate using NOWAIT without an IN...MODE clause.

Another possibility is to disallow just the single case
LOCK tablename NOWAIT
ie, you can write NOWAIT if you include *either* the object type
or the IN...MODE clause. This is not too hard as far as the grammar
is concerned, but I'm not exactly sure how to document it.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> Another possibility is to disallow just the single case
> LOCK tablename NOWAIT
> ie, you can write NOWAIT if you include *either* the object type
> or the IN...MODE clause. This is not too hard as far as the grammar
> is concerned, but I'm not exactly sure how to document it.

I don't see anything better than documenting it using 2 extra lines:

LOCK [ TABLE ] [ ONLY ] name [, ...] [ IN lockmode MODE ]
LOCK TABLE tablename [ IN lockmode MODE ] [ NOWAIT ]
LOCK [ TABLE ] [ ONLY ] tablename IN lockmode MODE [ NOWAIT ]

Ok it looks like a mess, but that's what it is :)

And every user with "LOCK tablename NOWAIT" in their code would have to
change that to "LOCK TABLE tablename NOWAIT". Is there no way to reduce
that to only be a problem with tables named the same as the new objects
we want to add support for?

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
>> It's a major undertaking trying to write software that runs against
>> PostgreSQL for more than one release. We should be making that easier,
>> not harder.
>
> None of the proposals would make it impossible to write a LOCK statement
> that works on all available releases, [....]

+1 for this as a nice guideline/philosophy for syntax changes in general.

Personally I don't mind changing a few SQL statements when I upgrade
to a new release; but it sure is nice if there's at least some syntax
that works on both a current and previous release.

On Sun, 2011-01-16 at 16:30 -0800, Ron Mayer wrote:
> Tom Lane wrote:
> > Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> >> It's a major undertaking trying to write software that runs against
> >> PostgreSQL for more than one release. We should be making that easier,
> >> not harder.
> >
> > None of the proposals would make it impossible to write a LOCK statement
> > that works on all available releases, [....]
>
> +1 for this as a nice guideline/philosophy for syntax changes in general.

Thanks Tom, appreciate it.

> Personally I don't mind changing a few SQL statements when I upgrade
> to a new release; but it sure is nice if there's at least some syntax
> that works on both a current and previous release.

Yes, changing to a form of syntax that is then both backwards and
forwards compatible is a good solution.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services