Thread: Assertion failure on UNLOGGED VIEW and SEQUENCE
UNLOGGED is defigned in OptTemp in gram.y, so the parser accepts
CREATE UNLOGGED VIEW and CREATE UNLOGGED SEQUENCE, but they are
actually not supported.
=# CREATE UNLOGGED VIEW uv AS SELECT 1;
TRAP: FailedAssertion("!(relkind == 'r' || relkind == 't')", File:
"heap.c", Line: 1246)
=# CREATE UNLOGGED SEQUENCE us;
TRAP: FailedAssertion("!(relkind == 'r' || relkind == 't')", File:
"heap.c", Line: 1246)
The most easiest fix would be preventing them in parser level.
--
Itagaki Takahiro
On Fri, Feb 18, 2011 at 6:42 AM, Itagaki Takahiro
<itagaki.takahiro@gmail.com> wrote:
> UNLOGGED is defigned in OptTemp in gram.y, so the parser accepts
> CREATE UNLOGGED VIEW and CREATE UNLOGGED SEQUENCE, but they are
> actually not supported.
>
> =# CREATE UNLOGGED VIEW uv AS SELECT 1;
> TRAP: FailedAssertion("!(relkind == 'r' || relkind == 't')", File:
> "heap.c", Line: 1246)
>
> =# CREATE UNLOGGED SEQUENCE us;
> TRAP: FailedAssertion("!(relkind == 'r' || relkind == 't')", File:
> "heap.c", Line: 1246)
>
> The most easiest fix would be preventing them in parser level.
Well, that sucks. I had intended for that to be disallowed at the
parser level, but obviously I fail. It seems that disallowing this in
the parser will require duplicating the OptTemp production. Or
alternatively we can just add an error check to the CREATE VIEW and
CREATE SEQUENCE productions, i.e.
if ($4 == RELPERSISTENCE_UNLOGGED) ereport(ERROR, ...);
I am somewhat leaning toward the latter option, to avoid unnecessarily
bloating the size of the parser tables, but I can do it the other way
if people prefer.
In scrutinizing this code again, I notice that I accidentally added
the ability to create an unlogged table using SELECT INTO, as in
"SELECT 1 INTO UNLOGGED foo", just as you can also do "SELECT 1 INTO
TEMP foo". I don't see any particular reason to disallow that, but I
should probably update the documentation.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Feb 18, 2011 at 6:42 AM, Itagaki Takahiro
>> The most easiest fix would be preventing them in parser level.
> Well, that sucks. I had intended for that to be disallowed at the
> parser level, but obviously I fail. It seems that disallowing this in
> the parser will require duplicating the OptTemp production. Or
> alternatively we can just add an error check to the CREATE VIEW and
> CREATE SEQUENCE productions, i.e.
> if ($4 == RELPERSISTENCE_UNLOGGED)
> ereport(ERROR, ...);
> I am somewhat leaning toward the latter option, to avoid unnecessarily
> bloating the size of the parser tables, but I can do it the other way
> if people prefer.
If by the first option you mean causing the failure report to be "syntax
error" rather than anything more specific, then I agree that option
sucks. I'd actually vote for putting the error test somewhere in
statement execution code, well downstream of gram.y. The parser's job
is to produce a parse tree, not to encapsulate knowledge about which
combinations of options are supported.
regards, tom lane
On Fri, Feb 18, 2011 at 10:02 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > If by the first option you mean causing the failure report to be "syntax > error" rather than anything more specific, then I agree that option > sucks. I'd actually vote for putting the error test somewhere in > statement execution code, well downstream of gram.y. The parser's job > is to produce a parse tree, not to encapsulate knowledge about which > combinations of options are supported. OK. Proposed patch attached. It looks to me like an unlogged view is inherently nonsensical, whereas an unlogged sequence is sensible but we don't implement it (and may never implement it, absent some evidence that the overhead of WAL logging sequence changes is worth getting excited about), so I wrote the error messages to reflect that distinction. I also added a couple of matching regression tests, and documented that UNLOGGED works with SELECT INTO. I put the check for views in DefineView adjacent to the other check that already cares about relpersistence, and I put the one in DefineSequence to match, at the top for lack of any compelling theory of where it ought to go. I looked at stuffing it all the way down into DefineRelation but that looks like it would require some other rejiggering of existing logic and assertions, which seems pointless and potentially prone to breaking things. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
Robert Haas <robertmhaas@gmail.com> writes:
> OK. Proposed patch attached. It looks to me like an unlogged view is
> inherently nonsensical, whereas an unlogged sequence is sensible but
> we don't implement it (and may never implement it, absent some
> evidence that the overhead of WAL logging sequence changes is worth
> getting excited about), so I wrote the error messages to reflect that
> distinction. I also added a couple of matching regression tests, and
> documented that UNLOGGED works with SELECT INTO. I put the check for
> views in DefineView adjacent to the other check that already cares
> about relpersistence, and I put the one in DefineSequence to match, at
> the top for lack of any compelling theory of where it ought to go. I
> looked at stuffing it all the way down into DefineRelation but that
> looks like it would require some other rejiggering of existing logic
> and assertions, which seems pointless and potentially prone to
> breaking things.
Regression tests for this seem pretty pointless (ie, a waste of cycles
forevermore). +1 for where you put the tests, but I don't think
ERRCODE_SYNTAX_ERROR is an appropriate errcode. I'd go with
FEATURE_NOT_SUPPORTED for both, I think. Also, it might be worth
putting some of the above justification into the comments, eg
/* Unlogged sequences are not implemented --- not clear if useful */
versus
/* Unlogged views are pretty nonsensical */
rather than duplicate comments describing non-duplicate cases.
regards, tom lane
On Fri, Feb 18, 2011 at 2:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> OK. Proposed patch attached. It looks to me like an unlogged view is >> inherently nonsensical, whereas an unlogged sequence is sensible but >> we don't implement it (and may never implement it, absent some >> evidence that the overhead of WAL logging sequence changes is worth >> getting excited about), so I wrote the error messages to reflect that >> distinction. I also added a couple of matching regression tests, and >> documented that UNLOGGED works with SELECT INTO. I put the check for >> views in DefineView adjacent to the other check that already cares >> about relpersistence, and I put the one in DefineSequence to match, at >> the top for lack of any compelling theory of where it ought to go. I >> looked at stuffing it all the way down into DefineRelation but that >> looks like it would require some other rejiggering of existing logic >> and assertions, which seems pointless and potentially prone to >> breaking things. > > Regression tests for this seem pretty pointless (ie, a waste of cycles > forevermore). +1 for where you put the tests, but I don't think > ERRCODE_SYNTAX_ERROR is an appropriate errcode. I'd go with > FEATURE_NOT_SUPPORTED for both, I think. I hesitate to use FEATURE_NOT_SUPPORTED for something that's nonsensical anyway. I picked SYNTAX_ERROR after some scrutiny of what I believe to be parallel cases, such as EXPLAIN (FOO) SELECT 1 and CREATE TABLE t AS SELECT 1 INTO me. > Also, it might be worth > putting some of the above justification into the comments, eg > > /* Unlogged sequences are not implemented --- not clear if useful */ > > versus > > /* Unlogged views are pretty nonsensical */ > > rather than duplicate comments describing non-duplicate cases. Good idea. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Feb 18, 2011 at 2:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> +1 for where you put the tests, but I don't think
>> ERRCODE_SYNTAX_ERROR is an appropriate errcode. I'd go with
>> FEATURE_NOT_SUPPORTED for both, I think.
> I hesitate to use FEATURE_NOT_SUPPORTED for something that's
> nonsensical anyway. I picked SYNTAX_ERROR after some scrutiny of what
> I believe to be parallel cases, such as EXPLAIN (FOO) SELECT 1 and
> CREATE TABLE t AS SELECT 1 INTO me.
[ shrug... ] I don't care enough to argue about it.
regards, tom lane