Re: [PATCH v1] Show whether tables are logged in \dt+

On Fri, 26 Apr 2019 at 14:49, Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com> wrote:
>
> On Wed, 24 Apr 2019 at 10:30, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> wrote:
> >
> >
> > Hello David,
> >
> > >>> I noticed that there wasn't a bulk way to see table logged-ness in psql,
> > >>> so I made it part of \dt+.
> > >>
> > >> Applies, compiles, works for me.
> > >>
> > >> ISTM That temporary-ness is not shown either. Maybe the persistence column
> > >> should be shown as is?
> > >
> > > Temporariness added, but not raw.
> >
> > Ok, it is better like this way.
> >
> > >> Tests?
> > >
> > > Included, but they're not stable for temp tables. I'm a little stumped
> > > as to how to either stabilize them or test some other way.
> >
> > Hmmm. First there is the username which appears, so there should be a
> > dedicated user for the test.
> >
> > I'm unsure how to work around the temporary schema number, which is
> > undeterministic with parallel execution it. I'm afraid the only viable
> > approach is not to show temporary tables, too bad:-(
> >
> > >> Doc?
> > >
> > > What further documentation does it need?
> >
> > Indeed, there is no precise doc, so nothing to update :-)/:-(
> >
> >
> > Maybe you could consider adding a case for prior 9.1 version, something
> > like:
> > ... case c.relistemp then 'temporary' else 'permanent' end as ...
> >
> >
> I was reviewing this patch and found a bug,
>
> create table t (i int);
> create index idx on t(i);
> \di+
> psql: print.c:3452: printQuery: Assertion `opt->translate_columns ==
> ((void *)0) || opt->n_translate_columns >= cont.ncolumns' failed.

Looking into this further, apparently the position of

if (verbose)
{
+ /*
+ * Show whether the table is permanent, temporary, or unlogged.
+ */
+ if (pset.sversion >= 91000)
+ appendPQExpBuffer(&buf,
+ ",\n case c.relpersistence when 'p' then 'permanent' when 't'
then 'temporary' when 'u' then 'unlogged' else 'unknown' end as
\"%s\"",
+ gettext_noop("Persistence"));

is not right, it is being called for indexes with verbose option also.
There should be an extra check for it being not called for index case.
Something like,
if (verbose)
{
/*
* Show whether the table is permanent, temporary, or unlogged.
*/
if (!showIndexes)
if (pset.sversion >= 91000)
appendPQExpBuffer(&buf,
",\n case c.relpersistence when 'p' then 'permanent' when 't' then
'temporary' when 'u' then 'unlogged' else 'unknown' end as \"%s\"",
gettext_noop("Persistence"));

Not sure, how do modify it in a more neat way.

--
Regards,
Rafia Sabih