Обсуждение: minor gripe about lax reloptions parsing for views

Does this bother anyone else:

CREATE INDEX uses an amoptions parser specific for the index type and, at least for btree, rejects relation options
fromthe "toast" namespace: 

+-- Bad reloption for index draws an error
+CREATE INDEX idx ON test_tbl USING btree (i) WITH (toast.nonsense=insanity);
+ERROR:  unrecognized parameter namespace "toast"

No so for CREATE VIEW, which shares logic with CREATE TABLE:

+-- But not for views, where "toast" namespace relopts are ignored
+CREATE VIEW nonsense_1 WITH (toast.nonsense=insanity, toast.foo="bar baz")
+   AS SELECT * FROM test_tbl;
+SELECT relname, reloptions FROM pg_class WHERE relname = 'nonsense_1';
+  relname   | reloptions
+------------+------------
+ nonsense_1 |
+(1 row)
+
+-- Well-formed but irrelevant toast options are also silently ignored
+CREATE VIEW vac_opts_1 WITH (toast.autovacuum_enabled=false)
+   AS SELECT * FROM test_tbl;
+SELECT relname, reloptions FROM pg_class WHERE relname = 'vac_opts_1';
+  relname   | reloptions
+------------+------------
+ vac_opts_1 |
+(1 row)

So far as I can see, this does no harm other than to annoy me.  It might confuse new users, though, as changing to a
MATERIALIZEDVIEW makes the toast options relevant, but the user feedback for the command is no different: 

+-- But if we upgrade to a materialized view, they are not ignored, but
+-- they attach to the toast table, not the view, so users might not notice
+-- the difference
+CREATE MATERIALIZED VIEW vac_opts_2 WITH (toast.autovacuum_enabled=false)
+   AS SELECT * FROM test_tbl;
+SELECT relname, reloptions FROM pg_class WHERE relname = 'vac_opts_2';
+  relname   | reloptions
+------------+------------
+ vac_opts_2 |
+(1 row)
+
+-- They can find the difference if they know where to look
+SELECT rel.relname, toast.relname, toast.reloptions
+   FROM pg_class rel LEFT JOIN pg_class toast ON rel.reltoastrelid = toast.oid
+   WHERE rel.relname IN ('nonsense_1', 'vac_opts_1', 'vac_opts_2');
+  relname   |    relname     |         reloptions
+------------+----------------+----------------------------
+ nonsense_1 |                |
+ vac_opts_1 |                |
+ vac_opts_2 | pg_toast_19615 | {autovacuum_enabled=false}
+(3 rows)

The solution is simple enough:  stop using HEAP_RELOPT_NAMESPACES when parsing reloptions for views and instead create
aVIEW_RELOPT_NAMESPACES array which does not include "toast". 

I've already fixed this, mixed into some other work.  I'll pull it out as its own patch if there is any interest.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2021-Sep-30, Mark Dilger wrote:

> The solution is simple enough:  stop using HEAP_RELOPT_NAMESPACES when
> parsing reloptions for views and instead create a
> VIEW_RELOPT_NAMESPACES array which does not include "toast".

It seems a reasonable (non-backpatchable) change to me.

> I've already fixed this, mixed into some other work.  I'll pull it out
> as its own patch if there is any interest.

Yeah.

I suppose you'll need a new routine that returns the namespace array to
use based on relkind.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Nunca confiaré en un traidor.  Ni siquiera si el traidor lo he creado yo"
(Barón Vladimir Harkonnen)

> On Oct 1, 2021, at 6:15 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Sep-30, Mark Dilger wrote:
>
>> The solution is simple enough:  stop using HEAP_RELOPT_NAMESPACES when
>> parsing reloptions for views and instead create a
>> VIEW_RELOPT_NAMESPACES array which does not include "toast".
>
> It seems a reasonable (non-backpatchable) change to me.

I agree.  It's neither important enough to be back-patched nor completely non-breaking.  Somebody could be passing
bogusreloptions and relying on the parser to ignore them. 

>> I've already fixed this, mixed into some other work.  I'll pull it out
>> as its own patch if there is any interest.
>
> Yeah.
>
> I suppose you'll need a new routine that returns the namespace array to
> use based on relkind.

The patch does it this way.  The new routine can just return NULL for relkinds that don't accept "toast" as an option
namespace. We don't need to create the VIEW_RELOPT_NAMESPACES array mentioned upthread. 

The patch changes the docs for index storage option "fillfactor".  The existing documentation imply that all index
methodssupport this parameter, but in truth built-in methods brin and gin do not, and we should not imply anything
aboutwhat non-built-in methods do. 

The changes to create_view.sql demonstrate what the patch has fixed.

The other changes to regression tests provide previously missing test coverage of storage options for which the
behavioris unchanged.  I prefer to have coverage, but if the committer who picks this up disagrees, those changes could
justbe ignored.  I'd also be happy to remove them and repost if the committer prefers. 




—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> On Oct 1, 2021, at 12:34 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>
> The patch does it this way.

A rebased patch is attached.



—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2021-Dec-21, Mark Dilger wrote:

> Rebased patch attached:

These tests are boringly repetitive.  Can't we have something like a
nested loop, with AMs on one and reloptions on the other, where each
reloption is tried on each AM and an exception block to report the
failure or success for each case?  Maybe have the list of AMs queried
from pg_am with hardcoded additions if needed?

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"Ninguna manada de bestias tiene una voz tan horrible como la humana" (Orual)

The patch is currently not applying.

And it looks like there hasn't been any discussion since Alvaro's
comments last december. I'm marking the patch Returned with Feedback.

On Fri, 24 Dec 2021 at 16:49, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Dec-21, Mark Dilger wrote:
>
> > Rebased patch attached:
>
> These tests are boringly repetitive.  Can't we have something like a
> nested loop, with AMs on one and reloptions on the other, where each
> reloption is tried on each AM and an exception block to report the
> failure or success for each case?  Maybe have the list of AMs queried
> from pg_am with hardcoded additions if needed?
>
> --
> Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
> "Ninguna manada de bestias tiene una voz tan horrible como la humana" (Orual)
>
>


--
greg