Horton

Re: Pluggable Storage - Andres's take

Amit Khandekar <amitdkhan.pg@gmail.com>

View thread View on postgr.es
From: Amit Khandekar <amitdkhan.pg@gmail.com>
To: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, Haribabu Kommi <kommi.haribabu@gmail.com>, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>, Peter Geoghegan <pg@bowt.ie>, Alvaro Herrera <alvherre@2ndquadrant.com>, Alexander Korotkov <a.korotkov@postgrespro.ru>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2019-01-21T08:33:16Z
Lists: pgsql-hackers 
On Sun, 20 Jan 2019 at 22:46, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>
> > On Fri, Jan 18, 2019 at 11:22 AM Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
> >
> > --- a/src/test/regress/expected/copy2.out
> > +++ b/src/test/regress/expected/copy2.out
> > @@ -1,3 +1,4 @@
> > +\set HIDE_TABLEAM on
> >
> > I thought we wanted to avoid having to add this setting in individual
> > regression tests. Can't we do this in pg_regress as a common setting ?
>
> Yeah, you're probably right. Actually, I couldn't find anything that looks like
> "common settings", and so far I've placed it into psql_start_test as a psql
> argument. But not sure, maybe there is a better place.

Yeah, psql_start_test() looks good to me. pg_regress does not seem to
have it's own psqlrc file where we could have put this variable. May
be later on if we want to have more such variables, we could device
this infrastructure.

>
> > + /* Access method info */
> > + if (pset.sversion >= 120000 && verbose && tableinfo.relam != NULL &&
> > +    !(pset.hide_tableam && tableinfo.relam_is_default))
> > + {
> > +         printfPQExpBuffer(&buf, _("Access method: %s"),
> > fmtId(tableinfo.relam));
> >
> > So this will make psql hide the access method if it's same as the
> > default. I understand that this was kind of concluded in the other
> > thread "Displaying and dumping of table access methods". But IMHO, if
> > the hide_tableam is false, we should *always* show the access method,
> > regardless of the default value. I mean, we can make it simple : off
> > means never show table-access, on means always show table-access,
> > regardless of the default access method. And this also will work with
> > regression tests. If some regression test wants specifically to output
> > the access method, it can have a "\SET HIDE_TABLEAM off" command.
> >
> > If we hide the method if it's default, then for a regression test that
> > wants to forcibly show the table access method of all tables, it won't
> > show up for tables that have default access method.
>
> I can't imagine, what kind of test would need to forcibly show the table access
> method of all the tables? Even if you need to verify tableam for something,
> maybe it's even easier just to select it from pg_am?

Actually my statement is wrong, sorry. For a regression test that
wants to forcibly show table access for all tables, it just needs to
SET HIDE_TABLEAM to OFF. With your patch, if we set HIDE_TABLEAM to
OFF, it will *always* show the table access regardless of default
access method.

It is with HIDE_TABLEAM=ON that your patch hides the table access
conditionally (i.e. it shows when default value does not match). It's
in this case, that I feel we should *unconditionally* hide the table
access. Regression tests that use \d+ to show the table details might
not be interested specifically in table access method. But these will
fail if run with a modified default access method.

Besides, my general inclination is : keep the GUC behaviour simple;
and also,  it looks like we can keep the regression test output
consistent without having to have this conditional behaviour.

-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company

Commits

  1. Add default_table_access_method to postgresql.conf.sample.

  2. tableam: Avoid relying on relation size to determine validity of tids.

  3. tableam: Don't assume that every AM uses md.c style storage.

  4. Allow pg_class xid & multixid horizons to not be set.

  5. Fix slot type issue for fuzzy distance index scan over out-of-core table AM.

  6. tableam: comment and formatting fixes.

  7. table: docs: fix typos and grammar.

  8. tableam: basic documentation.

  9. Only allow heap in a number of contrib modules.

  10. tableam: Add table_finish_bulk_insert().

  11. tableam: sample scan.

  12. tableam: bitmap table scan.

  13. tableam: Move heap specific logic from estimate_rel_size below tableam.

  14. tableam: VACUUM and ANALYZE support.

  15. tableam: relation creation, VACUUM FULL/CLUSTER, SET TABLESPACE.

  16. tableam: Support for an index build's initial table scan(s).

  17. tableam: Add table_get_latest_tid, to wrap heap_get_latest_tid.

  18. tableam: Add helper for indexes to check if a corresponding table tuples exist.

  19. tableam: Add and use table_fetch_row_version().

  20. tableam: Use in CREATE TABLE AS and CREATE MATERIALIZED VIEW.

  21. Ensure sufficient alignment for ParallelTableScanDescData in BTShared.

  22. Don't reuse slots between root and partition in ON CONFLICT ... UPDATE.

  23. Use a virtual rather than a heap slot in two places where that suffices.

  24. Store tuples for EvalPlanQual in slots, rather than as HeapTuples.

  25. Use slots in trigger infrastructure, except for the actual invocation.

  26. Store table oid and tuple's tid in tuple slots directly.

  27. Allow to use HeapTupleData embedded in [Buffer]HeapTupleTableSlot.

  28. Add ExecStorePinnedBufferHeapTuple.

  29. Add ArchiveOpts to pass options to ArchiveEntry

  30. Rename RelationData.rd_amroutine to rd_indam.

  31. Rephrase references to "time qualification".

  32. Move remaining code from tqual.[ch] to heapam.h / heapam_visibility.c.

  33. Move generic snapshot related code from tqual.h to snapmgr.h.

  34. Remove superfluous tqual.h includes.

  35. Don't duplicate parallel seqscan shmem sizing logic in nbtree.

  36. Move vacuumlazy.c into access/heap.

  37. Support parallel bitmap heap scans.