This patch:

commit 35ad25ad66fa3999bbc0bb59ca13cef3d750fb07
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date: Sat Jul 26 19:15:35 2008 +0000

As noted by Andrew Gierth, there's really no need any more to force a junk
filter to be used when INSERT or SELECT INTO has a plan that returns raw
disk tuples. The virtual-tuple-slot optimizations that were put in place
awhile ago mean that ExecInsert has to do ExecMaterializeSlot, and that
already copies the tuple if it's raw (and does so more efficiently than
a junk filter, too). So get rid of that logic. This in turn means that
we can throw away ExecMayReturnRawTuples, which wasn't used for any other
purpose, and was always a kluge anyway.

In passing, move a couple of SELECT-INTO-specific fields out of EState
and into the private state of the SELECT INTO DestReceiver, as was foreseen
in an old comment there. Also make intorel_receive use ExecMaterializeSlot
not ExecCopySlotTuple, for consistency with ExecInsert and to possibly save
a tuple copy step in some cases.
made this test case crash:

CREATE TABLE xtable (padding char(2000)) WITH OIDS;
INSERT INTO xtable VALUES('1');
ALTER TABLE xtable SET WITHOUT OIDS;
INSERT INTO xtable (SELECT * FROM xtable);

with assertion failure:

TRAP: FailedAssertion("!(!(tup->t_data->t_infomask & 0x0008))", File:
"heapam.c", Line: 1782)

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Search Discussions

  • Alvaro Herrera at Nov 5, 2008 at 8:32 pm

    Heikki Linnakangas wrote:

    made this test case crash:

    CREATE TABLE xtable (padding char(2000)) WITH OIDS;
    INSERT INTO xtable VALUES('1');
    ALTER TABLE xtable SET WITHOUT OIDS;
    INSERT INTO xtable (SELECT * FROM xtable);

    with assertion failure:

    TRAP: FailedAssertion("!(!(tup->t_data->t_infomask & 0x0008))", File:
    "heapam.c", Line: 1782)
    I think the fix is to just remove the Assert() ...

    --
    Alvaro Herrera http://www.CommandPrompt.com/
    The PostgreSQL Company - Command Prompt, Inc.
  • Heikki Linnakangas at Nov 5, 2008 at 8:45 pm

    Alvaro Herrera wrote:
    Heikki Linnakangas wrote:
    made this test case crash:

    CREATE TABLE xtable (padding char(2000)) WITH OIDS;
    INSERT INTO xtable VALUES('1');
    ALTER TABLE xtable SET WITHOUT OIDS;
    INSERT INTO xtable (SELECT * FROM xtable);

    with assertion failure:

    TRAP: FailedAssertion("!(!(tup->t_data->t_infomask & 0x0008))", File:
    "heapam.c", Line: 1782)
    That line number is wrong on CVS HEAD, BTW. I think I copy-pasted that
    from an old checkout. It's really:

    TRAP: FailedAssertion("!(!(tup->t_data->t_infomask & 0x0008))", File:
    "heapam.c", Line: 1855)
    I think the fix is to just remove the Assert() ...
    I don't think we want to insert tuples with OIDs to a table after SET
    WITHOUT OIDS. It would be a waste of space. And bizarre.

    --
    Heikki Linnakangas
    EnterpriseDB http://www.enterprisedb.com
  • Tom Lane at Nov 5, 2008 at 11:39 pm

    Heikki Linnakangas writes:
    This patch:
    As noted by Andrew Gierth, there's really no need any more to force a junk
    filter to be used when INSERT or SELECT INTO has a plan that returns raw
    disk tuples.
    made this test case crash:
    CREATE TABLE xtable (padding char(2000)) WITH OIDS;
    INSERT INTO xtable VALUES('1');
    ALTER TABLE xtable SET WITHOUT OIDS;
    INSERT INTO xtable (SELECT * FROM xtable);
    Hmm, that's kinda ugly. The real reason there's a problem, IMHO,
    is that the table contains tuples that don't match the rowtype
    specification. We've tried to skate around this and pretend that
    SET WITHOUT OIDS is cost-free, but it really isn't. I think this
    bug needs to be regarded as a member of a class of probable bugs,
    not an isolated error.

    Could we get away with turning SET WITHOUT OIDS into a table-rewriting
    operation that physically gets rid of the OIDs? The default has been
    no-oids for long enough that I'm not convinced that we need to risk more
    bugs in the name of keeping it a low-cost operation. (I note that we
    could then also support SET WITH OIDS with about the same infrastructure.)

    The alternative would probably be to treat a dropped OID column more
    like a dropped user column, including an explicit mark in the catalogs
    that "this table used to have OIDs" and special-casing all over the
    place. Doesn't seem attractive.

    regards, tom lane
  • KaiGai Kohei at Nov 27, 2008 at 4:40 am

    Heikki Linnakangas wrote:
    This patch:
    commit 35ad25ad66fa3999bbc0bb59ca13cef3d750fb07
    Author: Tom Lane <tgl@sss.pgh.pa.us>
    Date: Sat Jul 26 19:15:35 2008 +0000

    As noted by Andrew Gierth, there's really no need any more to force a junk
    filter to be used when INSERT or SELECT INTO has a plan that returns raw
    disk tuples. The virtual-tuple-slot optimizations that were put in place
    awhile ago mean that ExecInsert has to do ExecMaterializeSlot, and that
    already copies the tuple if it's raw (and does so more efficiently than
    a junk filter, too). So get rid of that logic. This in turn means that
    we can throw away ExecMayReturnRawTuples, which wasn't used for any other
    purpose, and was always a kluge anyway.
    In passing, move a couple of SELECT-INTO-specific fields out of EState
    and into the private state of the SELECT INTO DestReceiver, as was foreseen
    in an old comment there. Also make intorel_receive use ExecMaterializeSlot
    not ExecCopySlotTuple, for consistency with ExecInsert and to possibly save
    a tuple copy step in some cases.
    made this test case crash:

    CREATE TABLE xtable (padding char(2000)) WITH OIDS;
    INSERT INTO xtable VALUES('1');
    ALTER TABLE xtable SET WITHOUT OIDS;
    INSERT INTO xtable (SELECT * FROM xtable);

    with assertion failure:

    TRAP: FailedAssertion("!(!(tup->t_data->t_infomask & 0x0008))", File:
    "heapam.c", Line: 1782)
    http://wiki.postgresql.org/wiki/PostgreSQL_8.4_Open_Items

    In addition, it can show us another unexpected behavior.

    * Before patch applied:
    postgres=# CREATE TABLE t1 (a int, b text) WITH OIDS;
    CREATE TABLE
    postgres=# INSERT INTO t1 VALUES (1,'aaa'), (2,'bbb'), (3,'ccc');
    INSERT 0 3
    postgres=# SELECT oid,* FROM t1;
    oid | a | b
    -------+---+-----
    16405 | 1 | aaa
    16406 | 2 | bbb
    16407 | 3 | ccc
    (3 rows)

    postgres=# INSERT INTO t1 (SELECT * FROM t1);
    INSERT 0 3
    postgres=# SELECT oid,* FROM t1;
    oid | a | b
    -------+---+-----
    16405 | 1 | aaa
    16406 | 2 | bbb
    16407 | 3 | ccc
    16405 | 1 | aaa
    16406 | 2 | bbb
    16407 | 3 | ccc
    (6 rows)

    The newly insered three tuples preserves its object identifier because
    the fetched tuples has its valid object identifier which means it does
    not need to assign a new one.

    The matter comes from that we cannot guess ahead whether the fetched
    tuple has object identifier field, or not. Thus, it is necessary to
    enforce to translate fetched tuples into the current proper rowtype
    on INSERT, UPDATE or SELECT INTO.

    If my understanding is correct, the following patch can fix the matters.

    ---------------------(cut here)---------------------

    *** src/backend/executor/execScan.c (revision 1244)
    --- src/backend/executor/execScan.c (working copy)
    ***************
    *** 243,250 ****
    * If the plan context requires a particular hasoid setting, then that has
    * to match, too.
    */
    ! if (ExecContextForcesOids(ps, &hasoid) &&
    ! hasoid != tupdesc->tdhasoid)
    return false;

    return true;
    --- 243,249 ----
    * If the plan context requires a particular hasoid setting, then that has
    * to match, too.
    */
    ! if (ExecContextForcesOids(ps, &hasoid))
    return false;

    return true;
    ---------------------(cut here)---------------------

    * After the patch applied:

    postgres=# CREATE TABLE t1 (a int, b text) WITH OIDS;
    CREATE TABLE
    postgres=# INSERT INTO t1 VALUES (1,'aaa'), (2,'bbb'), (3,'ccc');
    INSERT 0 3
    postgres=# SELECT oid,* FROM t1;
    oid | a | b
    -------+---+-----
    16435 | 1 | aaa
    16436 | 2 | bbb
    16437 | 3 | ccc
    (3 rows)

    postgres=# ALTER TABLE t1 SET WITHOUT OIDS;
    ALTER TABLE
    postgres=# INSERT INTO t1 (SELECT * FROM t1);
    INSERT 0 3
    postgres=# SELECT * FROM t1;
    a | b
    ---+-----
    1 | aaa
    2 | bbb
    3 | ccc
    1 | aaa
    2 | bbb
    3 | ccc
    (6 rows)

    * After patch applied:
    postgres=# CREATE TABLE t1 (a int, b text) WITH OIDS;
    CREATE TABLE
    postgres=# INSERT INTO t1 VALUES (1,'aaa'), (2,'bbb'), (3,'ccc');
    INSERT 0 3
    postgres=# SELECT oid,* FROM t1;
    oid | a | b
    -------+---+-----
    16420 | 1 | aaa
    16421 | 2 | bbb
    16422 | 3 | ccc
    (3 rows)

    postgres=# INSERT INTO t1 (SELECT * FROM t1);
    INSERT 0 3
    postgres=# SELECT oid,* FROM t1;
    oid | a | b
    -------+---+-----
    16420 | 1 | aaa
    16421 | 2 | bbb
    16422 | 3 | ccc
    16423 | 1 | aaa
    16424 | 2 | bbb
    16425 | 3 | ccc
    (6 rows)

    Thanks,
    --
    OSS Platform Development Division, NEC
    KaiGai Kohei <kaigai@ak.jp.nec.com>
  • Tom Lane at Nov 27, 2008 at 3:23 pm

    KaiGai Kohei writes:
    If my understanding is correct, the following patch can fix the matters.
    ! if (ExecContextForcesOids(ps, &hasoid) &&
    ! hasoid != tupdesc->tdhasoid)
    return false;
    --- 243,249 ----
    ! if (ExecContextForcesOids(ps, &hasoid))
    return false;
    This isn't fixing anything, it's just making the executor stick its
    head in the sand.

    regards, tom lane
  • KaiGai Kohei at Nov 28, 2008 at 5:21 am

    Tom Lane wrote:
    KaiGai Kohei <kaigai@ak.jp.nec.com> writes:
    If my understanding is correct, the following patch can fix the matters.
    ! if (ExecContextForcesOids(ps, &hasoid) &&
    ! hasoid != tupdesc->tdhasoid)
    return false;
    --- 243,249 ----
    ! if (ExecContextForcesOids(ps, &hasoid))
    return false;
    This isn't fixing anything, it's just making the executor stick its
    head in the sand.
    Sorry, it is unclear for me why it does not fix anything.

    In my understanding, the matter comes from the mixture of two kind of
    tuples. The one has object identifier, and the other don't have.
    It seems to me the current implementation assumes fetched tuples have
    proper rowtype which matches to the current table definition, however,
    the ALTER TABLE can break this assumption. It makes impossible to guess
    ahead whether fetched tuples have its object identifier, or not.
    Therefore, I thought we need something to enforce proper rowtype
    prior to when a tuple is delivered to ExecInsert() as a new one.
    The patch enforces ExecProject() when INSERT, UPDATE or SELECT INTO
    cases, so it enables to deliver a tuple with proper rowtype.

    In addition, what is the expected behavior in the following case?
    I felt it a bit strange one, so reported.
    ========
    postgres=# CREATE TABLE t1 (a int, b text) WITH OIDS;
    CREATE TABLE
    postgres=# INSERT INTO t1 VALUES (1,'aaa'), (2,'bbb'), (3,'ccc');
    INSERT 0 3
    postgres=# SELECT oid,* FROM t1;
    oid | a | b
    -------+---+-----
    16405 | 1 | aaa
    16406 | 2 | bbb
    16407 | 3 | ccc
    (3 rows)

    postgres=# INSERT INTO t1 (SELECT * FROM t1);
    INSERT 0 3
    postgres=# SELECT oid,* FROM t1;
    oid | a | b
    -------+---+-----
    16405 | 1 | aaa
    16406 | 2 | bbb
    16407 | 3 | ccc
    16405 | 1 | aaa <--- newly inserted tuples preserve the object
    16406 | 2 | bbb identifier of its source tuples, not a newly
    16407 | 3 | ccc assigned one.
    (6 rows)
    ========

    Thanks,
    --
    OSS Platform Development Division, NEC
    KaiGai Kohei <kaigai@ak.jp.nec.com>
  • Tom Lane at Nov 28, 2008 at 6:01 pm

    KaiGai Kohei writes:
    In my understanding, the matter comes from the mixture of two kind of
    tuples. The one has object identifier, and the other don't have.
    It seems to me the current implementation assumes fetched tuples have
    proper rowtype which matches to the current table definition, however,
    the ALTER TABLE can break this assumption.
    Right. And the way to fix that is to fix ALTER TABLE to not break the
    assumption. Otherwise we'll be putting band-aids in different parts
    of the system for years to come. As I said when the point came up
    originally, there is no reason to assume that this is a problem that
    affects only one or two places.

    regards, tom lane

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedNov 5, '08 at 8:04p
activeNov 28, '08 at 6:01p
posts8
users4
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2022 Grokbase