pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedfast defaults in heap_getattr vs heap_deform_tuple
14+ messages / 5 participants
[nested] [flat]
Commits from this thread:
2019-02-06 171e0418b0 297d627e07 Fix heap_getattr() handling of fast defaults.
* fast defaults in heap_getattr vs heap_deform_tuple
@ 2019-02-01 16:24 Andres Freund <andres@anarazel.de>
2019-02-01 22:49 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-02 13:35 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
0 siblings, 2 replies; 14+ messages in thread
From: Andres Freund @ 2019-02-01 16:24 UTC (permalink / raw)
To: pgsql-hackers@postgresql.org, Andrew Dunstan <andrew@dunslane.net>
Hi,
While working on the patch to slotify trigger.c I got somewhat confused
by the need to expand tuples in trigger.c:
static HeapTuple
GetTupleForTrigger(EState *estate,
EPQState *epqstate,
ResultRelInfo *relinfo,
ItemPointer tid,
LockTupleMode lockmode,
TupleTableSlot **newSlot)
{
...
if (HeapTupleHeaderGetNatts(tuple.t_data) < relation->rd_att->natts)
result = heap_expand_tuple(&tuple, relation->rd_att);
else
result = heap_copytuple(&tuple);
ReleaseBuffer(buffer);
There's no explanation why GetTupleForTrigger() needs expanding tuples,
but most other parts of postgres don't. Normally deforming ought to take
care of expanding missing attrs.
As far as I can tell, the reason that it's needed is that heap_gettar()
wasn't updated along the rest of the functions. heap_deform_tuple(),
heap_attisnull() etc look for missing attrs, but heap_getattr() doesn't.
That, to me, makes no sense. The reason that we see a difference in
regression test output is that composite_to_json() uses heap_getattr(),
if it used heap_deform_tuple (which'd be considerably faster), we'd get
the default value.
Am I missing something?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: fast defaults in heap_getattr vs heap_deform_tuple
2019-02-01 16:24 fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
@ 2019-02-01 22:49 ` Andres Freund <andres@anarazel.de>
2019-02-02 12:29 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-05 15:14 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
1 sibling, 2 replies; 14+ messages in thread
From: Andres Freund @ 2019-02-01 22:49 UTC (permalink / raw)
To: pgsql-hackers@postgresql.org, Andrew Dunstan <andrew@dunslane.net>
Hi,
On 2019-02-01 08:24:04 -0800, Andres Freund wrote:
> While working on the patch to slotify trigger.c I got somewhat confused
> by the need to expand tuples in trigger.c:
>
> static HeapTuple
> GetTupleForTrigger(EState *estate,
> EPQState *epqstate,
> ResultRelInfo *relinfo,
> ItemPointer tid,
> LockTupleMode lockmode,
> TupleTableSlot **newSlot)
> {
> ...
> if (HeapTupleHeaderGetNatts(tuple.t_data) < relation->rd_att->natts)
> result = heap_expand_tuple(&tuple, relation->rd_att);
> else
> result = heap_copytuple(&tuple);
> ReleaseBuffer(buffer);
>
> There's no explanation why GetTupleForTrigger() needs expanding tuples,
> but most other parts of postgres don't. Normally deforming ought to take
> care of expanding missing attrs.
>
> As far as I can tell, the reason that it's needed is that heap_gettar()
> wasn't updated along the rest of the functions. heap_deform_tuple(),
> heap_attisnull() etc look for missing attrs, but heap_getattr() doesn't.
>
> That, to me, makes no sense. The reason that we see a difference in
> regression test output is that composite_to_json() uses heap_getattr(),
> if it used heap_deform_tuple (which'd be considerably faster), we'd get
> the default value.
>
> Am I missing something?
Indeed, a hacky patch fixing heap_getattr makes the heap_expand_tuple()
in trigger.c unnecessary. Attached. I think we need to do something
along those lines.
Andrew, I think it'd be good to do a ground up review of the fast
defaults patch. There's been a fair number of issues in it, and this is
a another pretty fundamental issue.
Greetings,
Andres Freund
Attachments:
poc-fixme-heap_getattr.diff (text/x-diff, 2.1K) download
diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
index ed4549ca579..783b04a3cb9 100644
--- a/src/backend/access/common/heaptuple.c
+++ b/src/backend/access/common/heaptuple.c
@@ -71,8 +71,6 @@
#define VARLENA_ATT_IS_PACKABLE(att) \
((att)->attstorage != 'p')
-static Datum getmissingattr(TupleDesc tupleDesc, int attnum, bool *isnull);
-
/* ----------------------------------------------------------------
* misc support routines
@@ -82,7 +80,7 @@ static Datum getmissingattr(TupleDesc tupleDesc, int attnum, bool *isnull);
/*
* Return the missing value of an attribute, or NULL if there isn't one.
*/
-static Datum
+Datum
getmissingattr(TupleDesc tupleDesc,
int attnum, bool *isnull)
{
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 7b5896b98f9..b412c81d141 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -3412,10 +3412,15 @@ ltrmark:;
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
}
+#ifdef IAM_THE_WRONG_FIX
if (HeapTupleHeaderGetNatts(tuple.t_data) < relation->rd_att->natts)
result = heap_expand_tuple(&tuple, relation->rd_att);
else
result = heap_copytuple(&tuple);
+#else
+ result = heap_copytuple(&tuple);
+#endif
+
ReleaseBuffer(buffer);
return result;
diff --git a/src/include/access/htup_details.h b/src/include/access/htup_details.h
index 873a3015fc4..81546ab46c7 100644
--- a/src/include/access/htup_details.h
+++ b/src/include/access/htup_details.h
@@ -744,6 +744,10 @@ extern Datum fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
#endif /* defined(DISABLE_COMPLEX_MACRO) */
+extern Datum
+getmissingattr(TupleDesc tupleDesc,
+ int attnum, bool *isnull);
+
/* ----------------
* heap_getattr
*
@@ -765,8 +769,7 @@ extern Datum fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
( \
((attnum) > (int) HeapTupleHeaderGetNatts((tup)->t_data)) ? \
( \
- (*(isnull) = true), \
- (Datum)NULL \
+ getmissingattr(tupleDesc, attnum, isnull) \
) \
: \
fastgetattr((tup), (attnum), (tupleDesc), (isnull)) \
* Re: fast defaults in heap_getattr vs heap_deform_tuple
2019-02-01 16:24 fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-01 22:49 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
@ 2019-02-02 12:29 ` Andres Freund <andres@anarazel.de>
1 sibling, 0 replies; 14+ messages in thread
From: Andres Freund @ 2019-02-02 12:29 UTC (permalink / raw)
To: pgsql-hackers@postgresql.org, Andrew Dunstan <andrew@dunslane.net>
Him
On 2019-02-01 14:49:05 -0800, Andres Freund wrote:
> +#ifdef IAM_THE_WRONG_FIX
> if (HeapTupleHeaderGetNatts(tuple.t_data) < relation->rd_att->natts)
> result = heap_expand_tuple(&tuple, relation->rd_att);
> else
> result = heap_copytuple(&tuple);
> +#else
> + result = heap_copytuple(&tuple);
> +#endif
This was added in
commit 7636e5c60fea83a9f3cd2ad278c0819b98941c74
Author: Andrew Dunstan <andrew@dunslane.net>
Date: 2018-09-24 16:11:24 -0400
Fast default trigger and expand_tuple fixes
Ensure that triggers get properly filled in tuples for the OLD value.
Also fix the logic of detecting missing null values. The previous logic
failed to detect a missing null column before the first missing column
with a default. Fixing this has simplified the logic a bit.
Regression tests are added to test changes. This should ensure better
coverage of expand_tuple().
Original bug reports, and some code and test scripts from Tomas Vondra
Backpatch to release 11.
Unfortunately the commit doesn't reference the discussion. That appears
to have been at
https://postgr.es/m/224e4807-395d-9fc5-2934-d5f85130f1f0%402ndQuadrant.com
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: fast defaults in heap_getattr vs heap_deform_tuple
2019-02-01 16:24 fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-01 22:49 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
@ 2019-02-05 15:14 ` Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
2019-02-05 16:57 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
1 sibling, 1 reply; 14+ messages in thread
From: Andrew Dunstan @ 2019-02-05 15:14 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; pgsql-hackers
On 2/1/19 5:49 PM, Andres Freund wrote:
> Hi,
>
> On 2019-02-01 08:24:04 -0800, Andres Freund wrote:
>
> Andrew, I think it'd be good to do a ground up review of the fast
> defaults patch. There's been a fair number of issues in it, and this is
> a another pretty fundamental issue.
>
OK. Will try to organise it.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: fast defaults in heap_getattr vs heap_deform_tuple
2019-02-01 16:24 fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-01 22:49 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-05 15:14 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
@ 2019-02-05 16:57 ` Andres Freund <andres@anarazel.de>
2019-02-06 01:44 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Alvaro Herrera <alvherre@2ndquadrant.com>
2019-02-06 09:17 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
0 siblings, 2 replies; 14+ messages in thread
From: Andres Freund @ 2019-02-05 16:57 UTC (permalink / raw)
To: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>; +Cc: pgsql-hackers
Hi,
On 2019-02-05 10:14:48 -0500, Andrew Dunstan wrote:
>
> On 2/1/19 5:49 PM, Andres Freund wrote:
> > Hi,
> >
> > On 2019-02-01 08:24:04 -0800, Andres Freund wrote:
> >
> > Andrew, I think it'd be good to do a ground up review of the fast
> > defaults patch. There's been a fair number of issues in it, and this is
> > a another pretty fundamental issue.
> >
>
>
> OK. Will try to organise it.
Cool. Here's the patch that I, after some commit message polishing, plan
to commit to 11 and master to fix the issue at hand.
Greetings,
Andres Freund
Attachments:
0001-Fix-heap_getattr-handling-of-fast-defaults.patch (text/x-diff, 5.7K) download
From 5fef71e1ffa0ba6a12ca9db6e286221750f53dc2 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 4 Feb 2019 07:10:12 -0800
Subject: [PATCH] Fix heap_getattr() handling of fast defaults.
Previously heap_getattr() returned NULL for attributes with a fast
default value (c.f. 16828d5c0273), as it had no handling whatsoever
for that case.
A previous fix, 7636e5c60f, attempted to fix issues caused by this
oversight, but just expanding OLD tuples for triggers doesn't actually
solve the actual issue.
One known consequence of this bug is that the check for HOT updates
can return the wrong result, when a previously fast-default'ed column
is set to NULL.
Fix by handling heap_getattr(), remove now superfluous expansion in
GetTupleForTrigger().
Author: Andres Freund
Discussion: https://postgr.es/m/20190201162404.onngi77f26baem4g@alap3.anarazel.de
Backpatch: 11, where fast defaults were introduced
---
src/backend/access/common/heaptuple.c | 4 +---
src/backend/commands/trigger.c | 5 +---
src/include/access/htup_details.h | 7 +++---
src/test/regress/expected/fast_default.out | 27 ++++++++++++++++++++++
src/test/regress/sql/fast_default.sql | 20 ++++++++++++++++
5 files changed, 52 insertions(+), 11 deletions(-)
diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
index ed4549ca579..783b04a3cb9 100644
--- a/src/backend/access/common/heaptuple.c
+++ b/src/backend/access/common/heaptuple.c
@@ -71,8 +71,6 @@
#define VARLENA_ATT_IS_PACKABLE(att) \
((att)->attstorage != 'p')
-static Datum getmissingattr(TupleDesc tupleDesc, int attnum, bool *isnull);
-
/* ----------------------------------------------------------------
* misc support routines
@@ -82,7 +80,7 @@ static Datum getmissingattr(TupleDesc tupleDesc, int attnum, bool *isnull);
/*
* Return the missing value of an attribute, or NULL if there isn't one.
*/
-static Datum
+Datum
getmissingattr(TupleDesc tupleDesc,
int attnum, bool *isnull)
{
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 7b5896b98f9..0b245a613e0 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -3412,10 +3412,7 @@ ltrmark:;
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
}
- if (HeapTupleHeaderGetNatts(tuple.t_data) < relation->rd_att->natts)
- result = heap_expand_tuple(&tuple, relation->rd_att);
- else
- result = heap_copytuple(&tuple);
+ result = heap_copytuple(&tuple);
ReleaseBuffer(buffer);
return result;
diff --git a/src/include/access/htup_details.h b/src/include/access/htup_details.h
index 873a3015fc4..d2b46456f1c 100644
--- a/src/include/access/htup_details.h
+++ b/src/include/access/htup_details.h
@@ -764,10 +764,7 @@ extern Datum fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
((attnum) > 0) ? \
( \
((attnum) > (int) HeapTupleHeaderGetNatts((tup)->t_data)) ? \
- ( \
- (*(isnull) = true), \
- (Datum)NULL \
- ) \
+ getmissingattr(tupleDesc, attnum, isnull) \
: \
fastgetattr((tup), (attnum), (tupleDesc), (isnull)) \
) \
@@ -788,6 +785,8 @@ extern Datum nocachegetattr(HeapTuple tup, int attnum,
TupleDesc att);
extern Datum heap_getsysattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
bool *isnull);
+extern Datum getmissingattr(TupleDesc tupleDesc,
+ int attnum, bool *isnull);
extern HeapTuple heap_copytuple(HeapTuple tuple);
extern void heap_copytuple_with_tuple(HeapTuple src, HeapTuple dest);
extern Datum heap_copy_tuple_as_datum(HeapTuple tuple, TupleDesc tupleDesc);
diff --git a/src/test/regress/expected/fast_default.out b/src/test/regress/expected/fast_default.out
index 40a15bd2d61..10bc5ff757c 100644
--- a/src/test/regress/expected/fast_default.out
+++ b/src/test/regress/expected/fast_default.out
@@ -770,6 +770,33 @@ SELECT * FROM vtype2;
2 | yyy
(2 rows)
+-- Ensure that defaults are checked when evaluating whether HOT update
+-- is possible, this was broken for a while:
+-- https://postgr.es/m/20190202133521.ylauh3ckqa7colzj%40alap3.anarazel.de
+BEGIN;
+CREATE TABLE t();
+INSERT INTO t DEFAULT VALUES;
+ALTER TABLE t ADD COLUMN a int DEFAULT 1;
+CREATE INDEX ON t(a);
+-- set column with a default 1 to NULL, due to a bug that wasn't
+-- noticed has heap_getattr buggily returned NULL for default columns
+UPDATE t SET a = NULL;
+-- verify that index and non-index scans show the same result
+SET LOCAL enable_seqscan = true;
+SELECT * FROM t WHERE a IS NULL;
+ a
+---
+
+(1 row)
+
+SET LOCAL enable_seqscan = false;
+SELECT * FROM t WHERE a IS NULL;
+ a
+---
+
+(1 row)
+
+ROLLBACK;
-- cleanup
DROP TABLE vtype;
DROP TABLE vtype2;
diff --git a/src/test/regress/sql/fast_default.sql b/src/test/regress/sql/fast_default.sql
index 0f65a79c7fd..4589b9e58d1 100644
--- a/src/test/regress/sql/fast_default.sql
+++ b/src/test/regress/sql/fast_default.sql
@@ -505,6 +505,26 @@ ALTER TABLE vtype2 ALTER COLUMN b TYPE varchar(20) USING b::varchar(20);
SELECT * FROM vtype2;
+-- Ensure that defaults are checked when evaluating whether HOT update
+-- is possible, this was broken for a while:
+-- https://postgr.es/m/20190202133521.ylauh3ckqa7colzj%40alap3.anarazel.de
+BEGIN;
+CREATE TABLE t();
+INSERT INTO t DEFAULT VALUES;
+ALTER TABLE t ADD COLUMN a int DEFAULT 1;
+CREATE INDEX ON t(a);
+-- set column with a default 1 to NULL, due to a bug that wasn't
+-- noticed has heap_getattr buggily returned NULL for default columns
+UPDATE t SET a = NULL;
+
+-- verify that index and non-index scans show the same result
+SET LOCAL enable_seqscan = true;
+SELECT * FROM t WHERE a IS NULL;
+SET LOCAL enable_seqscan = false;
+SELECT * FROM t WHERE a IS NULL;
+ROLLBACK;
+
+
-- cleanup
DROP TABLE vtype;
DROP TABLE vtype2;
--
2.18.0.rc2.dirty
* Re: fast defaults in heap_getattr vs heap_deform_tuple
2019-02-01 16:24 fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-01 22:49 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-05 15:14 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
2019-02-05 16:57 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
@ 2019-02-06 01:44 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-02-09 10:51 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
1 sibling, 1 reply; 14+ messages in thread
From: Alvaro Herrera @ 2019-02-06 01:44 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>; pgsql-hackers
On 2019-Feb-05, Andres Freund wrote:
> @@ -82,7 +80,7 @@ static Datum getmissingattr(TupleDesc tupleDesc, int attnum, bool *isnull);
> /*
> * Return the missing value of an attribute, or NULL if there isn't one.
> */
> -static Datum
> +Datum
> getmissingattr(TupleDesc tupleDesc,
> int attnum, bool *isnull)
This is a terrible name for an exported function -- let's change it
before it gets exported. Heck, even heap_getmissingattr() would be
better.
I notice that with this patch, heap_getattr() obtains a new Assert()
that the attr being fetched is no further than tupledesc->natts.
It previously just returned null for that case. Maybe we should change
it so that it returns null if an attr beyond end-of-array is fetched?
(I think in non-assert builds, it would dereference past the AttrMissing
array.)
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: fast defaults in heap_getattr vs heap_deform_tuple
2019-02-01 16:24 fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-01 22:49 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-05 15:14 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
2019-02-05 16:57 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-06 01:44 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Alvaro Herrera <alvherre@2ndquadrant.com>
@ 2019-02-09 10:51 ` Andres Freund <andres@anarazel.de>
0 siblings, 0 replies; 14+ messages in thread
From: Andres Freund @ 2019-02-09 10:51 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>; pgsql-hackers
Hi,
On 2019-02-05 22:44:38 -0300, Alvaro Herrera wrote:
> On 2019-Feb-05, Andres Freund wrote:
>
> > @@ -82,7 +80,7 @@ static Datum getmissingattr(TupleDesc tupleDesc, int attnum, bool *isnull);
> > /*
> > * Return the missing value of an attribute, or NULL if there isn't one.
> > */
> > -static Datum
> > +Datum
> > getmissingattr(TupleDesc tupleDesc,
> > int attnum, bool *isnull)
>
> This is a terrible name for an exported function -- let's change it
> before it gets exported. Heck, even heap_getmissingattr() would be
> better.
I don't really aggree. Note that the relevant datastructure is named
AttrMissing, and that the function isn't relevant for heap tuple. So I
don't really see what we'd otherwise name it? I think if we wanted to
improve this we should start with AttrMissing and not this function.
> I notice that with this patch, heap_getattr() obtains a new Assert()
> that the attr being fetched is no further than tupledesc->natts.
> It previously just returned null for that case. Maybe we should change
> it so that it returns null if an attr beyond end-of-array is fetched?
> (I think in non-assert builds, it would dereference past the AttrMissing
> array.)
Hm, it seems like there's plenty issues with accessing datums after the
end of the desc before this change, as well as after this change. Note
that slot_getattr() (in 11, it looks a bit different in master) already
calls getmissingattr(). And that's much more commonly used. Therefore
I'm disinclined to see this as a problem?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: fast defaults in heap_getattr vs heap_deform_tuple
2019-02-01 16:24 fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-01 22:49 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-05 15:14 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
2019-02-05 16:57 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
@ 2019-02-06 09:17 ` Andres Freund <andres@anarazel.de>
2019-02-06 10:25 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andrew Gierth <andrew@tao11.riddles.org.uk>
1 sibling, 1 reply; 14+ messages in thread
From: Andres Freund @ 2019-02-06 09:17 UTC (permalink / raw)
To: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>; +Cc: pgsql-hackers
On 2019-02-05 08:57:05 -0800, Andres Freund wrote:
> Hi,
>
> On 2019-02-05 10:14:48 -0500, Andrew Dunstan wrote:
> >
> > On 2/1/19 5:49 PM, Andres Freund wrote:
> > > Hi,
> > >
> > > On 2019-02-01 08:24:04 -0800, Andres Freund wrote:
> > >
> > > Andrew, I think it'd be good to do a ground up review of the fast
> > > defaults patch. There's been a fair number of issues in it, and this is
> > > a another pretty fundamental issue.
> > >
> >
> >
> > OK. Will try to organise it.
>
> Cool. Here's the patch that I, after some commit message polishing, plan
> to commit to 11 and master to fix the issue at hand.
And pushed.
- Andres
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: fast defaults in heap_getattr vs heap_deform_tuple
2019-02-01 16:24 fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-01 22:49 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-05 15:14 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
2019-02-05 16:57 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-06 09:17 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
@ 2019-02-06 10:25 ` Andrew Gierth <andrew@tao11.riddles.org.uk>
2019-02-06 11:39 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 14+ messages in thread
From: Andrew Gierth @ 2019-02-06 10:25 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>; pgsql-hackers
>>>>> "Andres" == Andres Freund <andres@anarazel.de> writes:
>> Cool. Here's the patch that I, after some commit message polishing,
>> plan to commit to 11 and master to fix the issue at hand.
Andres> And pushed.
Sorry I didn't spot this earlier, but... in the backpatch to v11, is the
expansion of the trigger tuple really unnecessary now? Since
heap_getattr is a macro, C-language triggers that aren't recompiled
won't see the default? So perhaps the expansion should be left in?
--
Andrew (irc:RhodiumToad)
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: fast defaults in heap_getattr vs heap_deform_tuple
2019-02-01 16:24 fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-01 22:49 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-05 15:14 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
2019-02-05 16:57 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-06 09:17 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-06 10:25 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andrew Gierth <andrew@tao11.riddles.org.uk>
@ 2019-02-06 11:39 ` Andres Freund <andres@anarazel.de>
2019-02-06 12:22 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Christoph Berg <myon@debian.org>
2019-02-09 10:54 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
0 siblings, 2 replies; 14+ messages in thread
From: Andres Freund @ 2019-02-06 11:39 UTC (permalink / raw)
To: Andrew Gierth <andrew@tao11.riddles.org.uk>; +Cc: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>; pgsql-hackers
Hi,
On 2019-02-06 10:25:56 +0000, Andrew Gierth wrote:
> >>>>> "Andres" == Andres Freund <andres@anarazel.de> writes:
>
> >> Cool. Here's the patch that I, after some commit message polishing,
> >> plan to commit to 11 and master to fix the issue at hand.
>
> Andres> And pushed.
>
> Sorry I didn't spot this earlier, but... in the backpatch to v11, is the
> expansion of the trigger tuple really unnecessary now? Since
> heap_getattr is a macro, C-language triggers that aren't recompiled
> won't see the default? So perhaps the expansion should be left in?
There was some IRC conversation about this:
RhodiumToad | andres: ping?
RhodiumToad | n/m, sent mail
andres | RhodiumToad: IDK, there's a lot of other heap_getattr calls, and most of them don't go
| through GetTupleForTrigger(). So extensions need to be recompiled either way.
andres | (I'll write that in a bit on list, in meeting now)
RhodiumToad | right, but those other calls were already broken.
RhodiumToad | whereas in a trigger, they'd have worked previously, but then break with your fix
Myon | the list of packages in Debian where heap_getattr appears is postgresql-11, pglogical,
| pgextwlist, citus, postgresql-plsh, wal2json, pg-cron, postgresql-rum
RhodiumToad | it's not an encouraged interface
I'm mildly disinclined to re-add the heap_expand_tuple, because it's not
the only case, and the extensions named above don't seem like they'd
specifically affected by the trigger path, but I'm not sure.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: fast defaults in heap_getattr vs heap_deform_tuple
2019-02-01 16:24 fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-01 22:49 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-05 15:14 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
2019-02-05 16:57 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-06 09:17 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-06 10:25 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andrew Gierth <andrew@tao11.riddles.org.uk>
2019-02-06 11:39 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
@ 2019-02-06 12:22 ` Christoph Berg <myon@debian.org>
1 sibling, 0 replies; 14+ messages in thread
From: Christoph Berg @ 2019-02-06 12:22 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Andrew Gierth <andrew@tao11.riddles.org.uk>; Andrew Dunstan <andrew.dunstan@2ndquadrant.com>; pgsql-hackers
> > Sorry I didn't spot this earlier, but... in the backpatch to v11, is the
> > expansion of the trigger tuple really unnecessary now? Since
> > heap_getattr is a macro, C-language triggers that aren't recompiled
> > won't see the default? So perhaps the expansion should be left in?
>
> Myon | the list of packages in Debian where heap_getattr appears is postgresql-11, pglogical,
> | pgextwlist, citus, postgresql-plsh, wal2json, pg-cron, postgresql-rum
>
> I'm mildly disinclined to re-add the heap_expand_tuple, because it's not
> the only case, and the extensions named above don't seem like they'd
> specifically affected by the trigger path, but I'm not sure.
I'm not following closely enough to say anything about which fix is
the best, but if this isn't a "recompile the world" case, we can get
the above list of packages rebuilt. It would be rather unpleasant to
have to schedule that for 50 packages, though.
Are rebuilt extension binaries compatible with older servers that do
not have the fix yet?
Christoph
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: fast defaults in heap_getattr vs heap_deform_tuple
2019-02-01 16:24 fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-01 22:49 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-05 15:14 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
2019-02-05 16:57 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-06 09:17 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-06 10:25 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andrew Gierth <andrew@tao11.riddles.org.uk>
2019-02-06 11:39 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
@ 2019-02-09 10:54 ` Andres Freund <andres@anarazel.de>
1 sibling, 0 replies; 14+ messages in thread
From: Andres Freund @ 2019-02-09 10:54 UTC (permalink / raw)
To: Andrew Gierth <andrew@tao11.riddles.org.uk>; +Cc: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>; pgsql-hackers
Hi,
On 2019-02-06 03:39:19 -0800, Andres Freund wrote:
> On 2019-02-06 10:25:56 +0000, Andrew Gierth wrote:
> > >>>>> "Andres" == Andres Freund <andres@anarazel.de> writes:
> >
> > >> Cool. Here's the patch that I, after some commit message polishing,
> > >> plan to commit to 11 and master to fix the issue at hand.
> >
> > Andres> And pushed.
> >
> > Sorry I didn't spot this earlier, but... in the backpatch to v11, is the
> > expansion of the trigger tuple really unnecessary now? Since
> > heap_getattr is a macro, C-language triggers that aren't recompiled
> > won't see the default? So perhaps the expansion should be left in?
>
> There was some IRC conversation about this:
>
> RhodiumToad | andres: ping?
> RhodiumToad | n/m, sent mail
> andres | RhodiumToad: IDK, there's a lot of other heap_getattr calls, and most of them don't go
> | through GetTupleForTrigger(). So extensions need to be recompiled either way.
> andres | (I'll write that in a bit on list, in meeting now)
> RhodiumToad | right, but those other calls were already broken.
> RhodiumToad | whereas in a trigger, they'd have worked previously, but then break with your fix
> Myon | the list of packages in Debian where heap_getattr appears is postgresql-11, pglogical,
> | pgextwlist, citus, postgresql-plsh, wal2json, pg-cron, postgresql-rum
> RhodiumToad | it's not an encouraged interface
>
> I'm mildly disinclined to re-add the heap_expand_tuple, because it's not
> the only case, and the extensions named above don't seem like they'd
> specifically affected by the trigger path, but I'm not sure.
I put the expansion back for 11. Seems harmless enough. I'm planning to
send a reply to the minor release info to -packagers informing people
that extensions compiled after the minor release might not be compatible
with older servers (because they depend on getmissingattr() which
previously wasn't an exposed function)
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: fast defaults in heap_getattr vs heap_deform_tuple
2019-02-01 16:24 fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
@ 2019-02-02 13:35 ` Andres Freund <andres@anarazel.de>
2019-02-02 14:48 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
1 sibling, 1 reply; 14+ messages in thread
From: Andres Freund @ 2019-02-02 13:35 UTC (permalink / raw)
To: pgsql-hackers@postgresql.org, Andrew Dunstan <andrew@dunslane.net>
Hi,
On 2019-02-01 08:24:04 -0800, Andres Freund wrote:
> While working on the patch to slotify trigger.c I got somewhat confused
> by the need to expand tuples in trigger.c:
>
> static HeapTuple
> GetTupleForTrigger(EState *estate,
> EPQState *epqstate,
> ResultRelInfo *relinfo,
> ItemPointer tid,
> LockTupleMode lockmode,
> TupleTableSlot **newSlot)
> {
> ...
> if (HeapTupleHeaderGetNatts(tuple.t_data) < relation->rd_att->natts)
> result = heap_expand_tuple(&tuple, relation->rd_att);
> else
> result = heap_copytuple(&tuple);
> ReleaseBuffer(buffer);
>
> There's no explanation why GetTupleForTrigger() needs expanding tuples,
> but most other parts of postgres don't. Normally deforming ought to take
> care of expanding missing attrs.
>
> As far as I can tell, the reason that it's needed is that heap_gettar()
> wasn't updated along the rest of the functions. heap_deform_tuple(),
> heap_attisnull() etc look for missing attrs, but heap_getattr() doesn't.
>
> That, to me, makes no sense. The reason that we see a difference in
> regression test output is that composite_to_json() uses heap_getattr(),
> if it used heap_deform_tuple (which'd be considerably faster), we'd get
> the default value.
>
> Am I missing something?
This breaks HOT (and probably also foreign keys), when fast default
columns are set to NULL, because HeapDetermineModifiedColumns() gets the
values with heap_getattr(), which returns a spurious NULL for the old
value (instead of the fast default value). That then would compare equal
to the new column value set to NULL.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: fast defaults in heap_getattr vs heap_deform_tuple
2019-02-01 16:24 fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-02 13:35 ` Re: fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
@ 2019-02-02 14:48 ` Andres Freund <andres@anarazel.de>
0 siblings, 0 replies; 14+ messages in thread
From: Andres Freund @ 2019-02-02 14:48 UTC (permalink / raw)
To: pgsql-hackers@postgresql.org, Andrew Dunstan <andrew@dunslane.net>
Hi,
On 2019-02-02 05:35:21 -0800, Andres Freund wrote:
> This breaks HOT (and probably also foreign keys), when fast default
> columns are set to NULL, because HeapDetermineModifiedColumns() gets the
> values with heap_getattr(), which returns a spurious NULL for the old
> value (instead of the fast default value). That then would compare equal
> to the new column value set to NULL.
Repro:
BEGIN;
CREATE TABLE t();
INSERT INTO t DEFAULT VALUES;
ALTER TABLE t ADD COLUMN a int default 1;
CREATE INDEX ON t(a);
UPDATE t SET a = NULL;
SET LOCAL enable_seqscan = true;
SELECT * FROM t WHERE a IS NULL;
SET LOCAL enable_seqscan = false;
SELECT * FROM t WHERE a IS NULL;
ROLLBACK;
output:
...
UPDATE 1
SET
┌────────┐
│ a │
├────────┤
│ (null) │
└────────┘
(1 row)
SET
┌───┐
│ a │
├───┤
└───┘
(0 rows)
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 14+ messages in thread
end of thread, other threads:[~2019-02-09 10:54 UTC | newest]
Thread overview: 14+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-02-01 16:24 fast defaults in heap_getattr vs heap_deform_tuple Andres Freund <andres@anarazel.de>
2019-02-01 22:49 ` Andres Freund <andres@anarazel.de>
2019-02-02 12:29 ` Andres Freund <andres@anarazel.de>
2019-02-05 15:14 ` Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
2019-02-05 16:57 ` Andres Freund <andres@anarazel.de>
2019-02-06 01:44 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-02-09 10:51 ` Andres Freund <andres@anarazel.de>
2019-02-06 09:17 ` Andres Freund <andres@anarazel.de>
2019-02-06 10:25 ` Andrew Gierth <andrew@tao11.riddles.org.uk>
2019-02-06 11:39 ` Andres Freund <andres@anarazel.de>
2019-02-06 12:22 ` Christoph Berg <myon@debian.org>
2019-02-09 10:54 ` Andres Freund <andres@anarazel.de>
2019-02-02 13:35 ` Andres Freund <andres@anarazel.de>
2019-02-02 14:48 ` Andres Freund <andres@anarazel.de>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox