pgsql: Remove dependency on HeapTuple from predicate locking functions.

Started by Thomas Munroover 6 years ago6 messagescomitters
Jump to latest
#1Thomas Munro
thomas.munro@gmail.com

Remove dependency on HeapTuple from predicate locking functions.

The following changes make the predicate locking functions more
generic and suitable for use by future access methods:

- PredicateLockTuple() is renamed to PredicateLockTID(). It takes
ItemPointer and inserting transaction ID instead of HeapTuple.

- CheckForSerializableConflictIn() takes blocknum instead of buffer.

- CheckForSerializableConflictOut() no longer takes HeapTuple or buffer.

Author: Ashwin Agrawal
Reviewed-by: Andres Freund, Kuntal Ghosh, Thomas Munro
Discussion: /messages/by-id/CALfoeiv0k3hkEb3Oqk=ziWqtyk2Jys1UOK5hwRBNeANT_yX+ng@mail.gmail.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/6f38d4dac381b5b8bead302a0b4f81761042cd25

Modified Files
--------------
src/backend/access/gin/ginbtree.c | 2 +-
src/backend/access/gin/ginfast.c | 2 +-
src/backend/access/gin/gininsert.c | 6 +-
src/backend/access/gist/gist.c | 2 +-
src/backend/access/hash/hashinsert.c | 2 +-
src/backend/access/heap/heapam.c | 125 ++++++++++++++++++++++++----
src/backend/access/heap/heapam_handler.c | 11 +--
src/backend/access/index/indexam.c | 4 +-
src/backend/access/nbtree/nbtinsert.c | 4 +-
src/backend/storage/lmgr/predicate.c | 137 ++++++++++---------------------
src/include/access/heapam.h | 2 +
src/include/storage/predicate.h | 9 +-
12 files changed, 176 insertions(+), 130 deletions(-)

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Thomas Munro (#1)
Re: pgsql: Remove dependency on HeapTuple from predicate locking functions.

Thomas Munro <tmunro@postgresql.org> writes:

Remove dependency on HeapTuple from predicate locking functions.

anole's not terribly pleased with this:

"heapam.c", line 9137: error #2118: a void function may not return a value
return CheckForSerializableConflictOut(relation, xid, snapshot);
^

regards, tom lane

#3Thomas Munro
thomas.munro@gmail.com
In reply to: Tom Lane (#2)
Re: pgsql: Remove dependency on HeapTuple from predicate locking functions.

On Tue, Jan 28, 2020 at 6:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Thomas Munro <tmunro@postgresql.org> writes:

Remove dependency on HeapTuple from predicate locking functions.

anole's not terribly pleased with this:

"heapam.c", line 9137: error #2118: a void function may not return a value
return CheckForSerializableConflictOut(relation, xid, snapshot);
^

Thanks. I pushed a fix.

Wow, HP C spits out a lot of warnings.

#4Tom Lane
tgl@sss.pgh.pa.us
In reply to: Thomas Munro (#3)
Re: pgsql: Remove dependency on HeapTuple from predicate locking functions.

Thomas Munro <thomas.munro@gmail.com> writes:

On Tue, Jan 28, 2020 at 6:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

anole's not terribly pleased with this:
"heapam.c", line 9137: error #2118: a void function may not return a value
return CheckForSerializableConflictOut(relation, xid, snapshot);

Thanks. I pushed a fix.
Wow, HP C spits out a lot of warnings.

It's pretty noisy, and most of 'em are useless :-(. But as for this
particular complaint, I don't really understand why gcc lets it slide.
The C99 standard is not exactly ambiguous about this:

6.8.6.4 The return statement

Constraints

[#1] A return statement with an expression shall not appear
in a function whose return type is void. A return statement
without an expression shall only appear in a function whose
return type is void.

There is absolutely no question that the original coding is illegal
per spec, and it isn't even a particularly useful shorthand; so why
can't we get even a warning about it?

regards, tom lane

#5Thomas Munro
thomas.munro@gmail.com
In reply to: Tom Lane (#4)
Re: pgsql: Remove dependency on HeapTuple from predicate locking functions.

On Wed, Jan 29, 2020 at 4:05 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Thomas Munro <thomas.munro@gmail.com> writes:

On Tue, Jan 28, 2020 at 6:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

anole's not terribly pleased with this:
"heapam.c", line 9137: error #2118: a void function may not return a value
return CheckForSerializableConflictOut(relation, xid, snapshot);

Thanks. I pushed a fix.
Wow, HP C spits out a lot of warnings.

It's pretty noisy, and most of 'em are useless :-(. But as for this
particular complaint, I don't really understand why gcc lets it slide.

Maybe because it's allowed in C++, and pretty harmless.

There is absolutely no question that the original coding is illegal
per spec, and it isn't even a particularly useful shorthand; so why
can't we get even a warning about it?

$ cat test.c
void f() {}
void g() { return f(); }
$ cc -c -Wall test.c
$ cc -c -Wpedantic test.c
test.c:2:12: warning: void function 'g' should not return void
expression [-Wpedantic]
void g() { return f(); }
^ ~~~
1 warning generated.

Many other constructs in PostgreSQL are rejected by that switch,
though, and I don't see a way to ask for just that one warning.

#6Tom Lane
tgl@sss.pgh.pa.us
In reply to: Thomas Munro (#5)
Re: pgsql: Remove dependency on HeapTuple from predicate locking functions.

Thomas Munro <thomas.munro@gmail.com> writes:

On Wed, Jan 29, 2020 at 4:05 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

There is absolutely no question that the original coding is illegal
per spec, and it isn't even a particularly useful shorthand; so why
can't we get even a warning about it?

$ cc -c -Wpedantic test.c
test.c:2:12: warning: void function 'g' should not return void
expression [-Wpedantic]
void g() { return f(); }
^ ~~~
1 warning generated.

Many other constructs in PostgreSQL are rejected by that switch,
though, and I don't see a way to ask for just that one warning.

Yeah, -Wpedantic is a little *too* pedantic I'm afraid. Oh well.

regards, tom lane