Обсуждение: [PATCH v1] Show whether tables are logged in \dt+
Folks, I noticed that there wasn't a bulk way to see table logged-ness in psql, so I made it part of \dt+. What say? Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Вложения
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? Also I'd suggest that the column should be displayed before the "description" column to keep the length-varying one last? > What say? Tests? Doc? -- Fabien.
On Tue, Apr 23, 2019 at 07:03:58AM +0200, Fabien COELHO 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. > Also I'd suggest that the column should be displayed before the > "description" column to keep the length-varying one last? Done. > > What say? > > 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. > Doc? What further documentation does it need? Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Вложения
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 ... -- Fabien.
On Wed, 24 Apr 2019 at 10:30, Fabien COELHO <coelho@cri.ensmp.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. -- Regards, Rafia Sabih
On Fri, 26 Apr 2019 at 14:49, Rafia Sabih <rafia.pghackers@gmail.com> wrote:
>
> On Wed, 24 Apr 2019 at 10:30, Fabien COELHO <coelho@cri.ensmp.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
On Fri, Apr 26, 2019 at 04:22:18PM +0200, Rafia Sabih wrote:
> On Fri, 26 Apr 2019 at 14:49, Rafia Sabih <rafia.pghackers@gmail.com> wrote:
> >
> > On Wed, 24 Apr 2019 at 10:30, Fabien COELHO <coelho@cri.ensmp.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.
I suspect that as this may get a little messier, but I've made it
fairly neat short of a major refactor.
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Вложения
Hello David,
Patch v3 applies, but compiles for me with a warning because the
indentation of the following size block has been changed:
describe.c: In function ‘listTables’:
describe.c:3705:7: warning: this ‘if’ clause does not guard...
[-Wmisleading-indentation]
else if (pset.sversion >= 80100)
^~
describe.c:3710:3: note: ...this statement, but the latter is misleadingly
indented as if it were guarded by the ‘if’
appendPQExpBuffer(&buf,
^~~~~~~~~~~~~~~~~
Make check fails because of my temp schema was numbered 4 instead of 3,
and I'm "fabien" rather than "shackle".
>>>>> 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:-(
The tests have not been fixed.
I think that they need a dedicated user to replace "shackle", and I'm
afraid that there temporary test schema instability cannot be easily fixed
at the "psql" level, but would require some kind of TAP tests instead if
it is to be checked. In the short term, do not.
I checked that the \di+ works, though. I've played with temporary views
and \dv as well.
I discovered that you cannot have temporary unlogged objects, nor
temporary or unlogged materialized views. Intuitively I'd have thought
that these features would be orthogonal, but they are not. Also I created
an unlogged table with a SERIAL which created a sequence. The table is
unlogged but the sequence is permanent, which is probably ok.
I only have packages down to pg 9.3, so I could not test prior 9.1. By
looking at the online documentation, is seems that relistemp appears in pg
8.4, so the corresponding extraction should be guarded by this version.
Before that, temporary objects existed but were identified indirectly,
possibly because they were stored in a temporary schema. I suggest not to
try to address cases prior 8.4.
--
Fabien.
On Sat, Apr 27, 2019 at 09:19:57AM +0200, Fabien COELHO wrote: > > Hello David, > > Patch v3 applies, but compiles for me with a warning because the indentation > of the following size block has been changed: > > describe.c: In function ‘listTables’: > describe.c:3705:7: warning: this ‘if’ clause does not guard... > [-Wmisleading-indentation] > else if (pset.sversion >= 80100) > ^~ > describe.c:3710:3: note: ...this statement, but the latter is misleadingly > indented as if it were guarded by the ‘if’ > appendPQExpBuffer(&buf, > ^~~~~~~~~~~~~~~~~ Fixed. > Make check fails because of my temp schema was numbered 4 instead of 3, and > I'm "fabien" rather than "shackle". I think the way forward is to test this with TAP rather than the fixed-string method. > > > > > > 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:-( > > The tests have not been fixed. > > I think that they need a dedicated user to replace "shackle", and I'm afraid > that there temporary test schema instability cannot be easily fixed at the > "psql" level, but would require some kind of TAP tests instead if it is to > be checked. In the short term, do not. Checks removed while I figure out a new TAP test. > I checked that the \di+ works, though. I've played with temporary views and > \dv as well. Great! > I discovered that you cannot have temporary unlogged objects, nor > temporary or unlogged materialized views. Intuitively I'd have > thought that these features would be orthogonal, but they are not. This seems like material for a different patch. > Also I created an unlogged table with a SERIAL which created a > sequence. The table is unlogged but the sequence is permanent, which > is probably ok. > I only have packages down to pg 9.3, so I could not test prior 9.1. > By looking at the online documentation, is seems that relistemp > appears in pg 8.4, so the corresponding extraction should be guarded > by this version. Before that, temporary objects existed but were > identified indirectly, possibly because they were stored in a > temporary schema. I suggest not to try to address cases prior 8.4. Done. Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Вложения
Hello David, Patch applies. There seems to be a compilation issue: describe.c:5974:1: error: expected declaration or statement at end of input } Also there is an added indentation problem: the size & description stuff have been moved left but it should still in the verbose case, and a } is missing after them, which fixes the compilation. >> Make check fails because of my temp schema was numbered 4 instead of 3, and >> I'm "fabien" rather than "shackle". > > I think the way forward is to test this with TAP rather than the > fixed-string method. Ok. > Checks removed while I figure out a new TAP test. >> I only have packages down to pg 9.3, so I could not test prior 9.1. >> By looking at the online documentation, is seems that relistemp >> appears in pg 8.4, so the corresponding extraction should be guarded >> by this version. Before that, temporary objects existed but were >> identified indirectly, possibly because they were stored in a >> temporary schema. I suggest not to try to address cases prior 8.4. > > Done. After some checking, I think that there is an issue with the version numbers: - 9.1 is 90100, not 91000 - 8.4 is 80400, not 84000 Also, it seems that describes builds queries with uppercase keywords, so probably the new additions should follow that style: case -> CASE (and also when then else end as…) -- Fabien.
On Sat, Apr 27, 2019 at 10:38:50PM +0200, Fabien COELHO wrote: > > Hello David, > > Patch applies. There seems to be a compilation issue: > > describe.c:5974:1: error: expected declaration or statement at end of > input > } This is in brown paper bag territory. Fixed. > > I think the way forward is to test this with TAP rather than the > > fixed-string method. > > Ok. I've sent a separate patch extracted from the one you sent which adds stdin to our TAP testing infrastructure. I hope it lands so it'll be simpler to add these tests in a future version of the patch. > > Checks removed while I figure out a new TAP test. > > > > I only have packages down to pg 9.3, so I could not test prior 9.1. > > > By looking at the online documentation, is seems that relistemp > > > appears in pg 8.4, so the corresponding extraction should be guarded > > > by this version. Before that, temporary objects existed but were > > > identified indirectly, possibly because they were stored in a > > > temporary schema. I suggest not to try to address cases prior 8.4. > > > > Done. > > After some checking, I think that there is an issue with the version > numbers: > - 9.1 is 90100, not 91000 > - 8.4 is 80400, not 84000 Another brown paper bag, now fixed. > Also, it seems that describes builds queries with uppercase > keywords, so probably the new additions should follow that style: > case -> CASE (and also when then else end as…) Done. Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Вложения
Not particularly on topic, but: including a patch version number in your
subject headings is pretty unfriendly IMO, because it breaks threading
for people whose MUAs do threading by matching up subject lines.
I don't actually see the point of the [PATCH] annotation at all, because
the thread is soon going to contain lots of messages with the same subject
line but no embedded patch. Like this one. So it's just noise with no
information content worth noticing.
regards, tom lane
Hello David, >> Patch applies. There seems to be a compilation issue: >> >> describe.c:5974:1: error: expected declaration or statement at end of >> input >> } > > This is in brown paper bag territory. Fixed. I do not understand why you move both size and description out of the verbose mode, it should be there only when under verbose? > I've sent a separate patch extracted from the one you sent which adds > stdin to our TAP testing infrastructure. I hope it lands so it'll be > simpler to add these tests in a future version of the patch. Why not. As I'm the one who wrote the modified function, probably I could have thought of providing an input. I'm not sure it is worth a dedicated submission, could go together with any commit that would use it. -- Fabien.
On Sun, Apr 28, 2019 at 01:14:01PM -0400, Tom Lane wrote: > Not particularly on topic, but: including a patch version number in your > subject headings is pretty unfriendly IMO, because it breaks threading > for people whose MUAs do threading by matching up subject lines. Thanks for letting me know about those MUAs. > I don't actually see the point of the [PATCH] annotation at all, > because the thread is soon going to contain lots of messages with > the same subject line but no embedded patch. Like this one. So > it's just noise with no information content worth noticing. OK Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On Sun, Apr 28, 2019 at 07:26:55PM +0200, Fabien COELHO wrote: > > Hello David, > > > > Patch applies. There seems to be a compilation issue: > > > > > > describe.c:5974:1: error: expected declaration or statement at end of > > > input > > > } > > > > This is in brown paper bag territory. Fixed. > > I do not understand why you move both size and description out of the > verbose mode, it should be there only when under verbose? My mistake. Fixed. > > I've sent a separate patch extracted from the one you sent which adds > > stdin to our TAP testing infrastructure. I hope it lands so it'll be > > simpler to add these tests in a future version of the patch. > > Why not. As I'm the one who wrote the modified function, probably I could > have thought of providing an input. I'm not sure it is worth a dedicated > submission, could go together with any commit that would use it. My hope is that this is seen as a bug fix and gets back-patched. Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Вложения
Hello David,
> My mistake. Fixed.
About v6: applies, compiles, make check ok.
Code is ok.
Maybe there could be a comment to tell that prior version are not
addressed, something like:
...
}
/* else do not bother guessing the temporary status on old version */
No tests, pending an added TAP test infrastructure for psql.
I have a question a out the index stuff: indexes seem to appear as entries
in pg_class, and ISTM that they can be temporary/unlogged/permanent as
attached to corresponding objects. So the guard is not very useful and it
could make sense to show the information on indexes as well.
After removing the guard:
postgres=# \dtmv+ *foo*
List of relations
┌───────────┬──────┬───────────────────┬────────┬─────────────┬─────────┬─────────────┐
│ Schema │ Name │ Type │ Owner │ Persistence │ Size │ Description │
├───────────┼──────┼───────────────────┼────────┼─────────────┼─────────┼─────────────┤
│ pg_temp_3 │ foo │ table │ fabien │ temporary │ 0 bytes │ │
│ public │ mfoo │ materialized view │ fabien │ permanent │ 0 bytes │ │
│ public │ ufoo │ table │ fabien │ unlogged │ 0 bytes │ │
└───────────┴──────┴───────────────────┴────────┴─────────────┴─────────┴─────────────┘
(3 rows)
postgres=# \di+ *foo*
List of relations
┌───────────┬───────────┬───────┬────────┬───────┬─────────────┬────────────┬─────────────┐
│ Schema │ Name │ Type │ Owner │ Table │ Persistence │ Size │ Description │
├───────────┼───────────┼───────┼────────┼───────┼─────────────┼────────────┼─────────────┤
│ pg_temp_3 │ foo_pkey │ index │ fabien │ foo │ temporary │ 8192 bytes │ │
│ public │ ufoo_pkey │ index │ fabien │ ufoo │ unlogged │ 16 kB │ │
│ public │ ufoou │ index │ fabien │ ufoo │ unlogged │ 16 kB │ │
└───────────┴───────────┴───────┴────────┴───────┴─────────────┴────────────┴─────────────┘
(3 rows)
Is there a special reason not to show it?
--
Fabien.
On Mon, Apr 29, 2019 at 08:48:17AM +0200, Fabien COELHO wrote: > > Hello David, > > > My mistake. Fixed. > > About v6: applies, compiles, make check ok. > > Code is ok. > > Maybe there could be a comment to tell that prior version are not addressed, > something like: > > ... > } > /* else do not bother guessing the temporary status on old version */ Did something like this. > No tests, pending an added TAP test infrastructure for psql. Right. > I have a question a out the index stuff: indexes seem to appear as entries > in pg_class, and ISTM that they can be temporary/unlogged/permanent as > attached to corresponding objects. So the guard is not very useful and it > could make sense to show the information on indexes as well. Done. Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Вложения
Hello David, Patch v7 applies, compiles, make check ok. No docs needed. No tests, pending some TAP infrastructure. I could no test with a version between 8.4 & 9.1. No further comments. Marked as ready. -- Fabien.
On Sat, 27 Apr 2019 at 06:18, David Fetter <david@fetter.org> wrote:
>
> On Fri, Apr 26, 2019 at 04:22:18PM +0200, Rafia Sabih wrote:
> > On Fri, 26 Apr 2019 at 14:49, Rafia Sabih <rafia.pghackers@gmail.com> wrote:
> > >
> > > On Wed, 24 Apr 2019 at 10:30, Fabien COELHO <coelho@cri.ensmp.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.
>
> I suspect that as this may get a little messier, but I've made it
> fairly neat short of a major refactor.
>
I found the following warning on the compilation,
describe.c: In function ‘listTables’:
describe.c:3705:7: warning: this ‘if’ clause does not guard...
[-Wmisleading-indentation]
else if (pset.sversion >= 80100)
^~
describe.c:3710:3: note: ...this statement, but the latter is
misleadingly indented as if it were guarded by the ‘if’
appendPQExpBuffer(&buf,
Talking of indentation, you might want to run pgindent once. Other
than that the patch looks good to me.
--
Regards,
Rafia Sabih
David Fetter <david@fetter.org> writes:
> [ v7-0001-Show-detailed-relation-persistence-in-dt.patch ]
I looked this over and had a few suggestions, as per attached v8:
* The persistence description values ought to be translatable, as
is the usual practice in describe.c. This is slightly painful
because it requires tweaking the translate_columns[] values in a
non-constant way, but it's not that bad.
* I dropped the "ELSE 'unknown'" bit in favor of just emitting NULL
if the persistence isn't recognized. This is the same way that the
table-type CASE just above does it, and I see no reason to be different.
Moreover, there are message-style-guidelines issues with what to print
if you do want to print something; "unknown" doesn't cut it.
* I also dropped the logic for pre-9.1 servers, because the existing
precedent in describeOneTableDetails() is that we only consider
relpersistence for >= 9.1, and I don't see a real good reason to
deviate from that. 9.0 and before are long out of support anyway.
If there aren't objections, I think v8 is committable.
regards, tom lane
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 1c770b4..0e0af5f 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3632,7 +3632,8 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
PQExpBufferData buf;
PGresult *res;
printQueryOpt myopt = pset.popt;
- static const bool translate_columns[] = {false, false, true, false, false, false, false};
+ int cols_so_far;
+ bool translate_columns[] = {false, false, true, false, false, false, false, false};
/* If tabtypes is empty, we default to \dtvmsE (but see also command.c) */
if (!(showTables || showIndexes || showViews || showMatViews || showSeq || showForeign))
@@ -3672,15 +3673,40 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
gettext_noop("partitioned index"),
gettext_noop("Type"),
gettext_noop("Owner"));
+ cols_so_far = 4;
if (showIndexes)
+ {
appendPQExpBuffer(&buf,
- ",\n c2.relname as \"%s\"",
+ ",\n c2.relname as \"%s\"",
gettext_noop("Table"));
+ cols_so_far++;
+ }
if (verbose)
{
/*
+ * Show whether a relation is permanent, temporary, or unlogged. Like
+ * describeOneTableDetails(), we consider that persistence emerged in
+ * v9.1, even though related concepts existed before.
+ */
+ if (pset.sversion >= 90100)
+ {
+ appendPQExpBuffer(&buf,
+ ",\n CASE c.relpersistence WHEN 'p' THEN '%s' WHEN 't' THEN '%s' WHEN 'u' THEN '%s' END
as\"%s\"",
+ gettext_noop("permanent"),
+ gettext_noop("temporary"),
+ gettext_noop("unlogged"),
+ gettext_noop("Persistence"));
+ translate_columns[cols_so_far] = true;
+ }
+
+ /*
+ * We don't bother to count cols_so_far below here, as there's no need
+ * to; this might change with future additions to the output columns.
+ */
+
+ /*
* As of PostgreSQL 9.0, use pg_table_size() to show a more accurate
* size of a table, including FSM, VM and TOAST tables.
*/
On 2019-Jul-02, Tom Lane wrote: > * The persistence description values ought to be translatable, as > is the usual practice in describe.c. This is slightly painful > because it requires tweaking the translate_columns[] values in a > non-constant way, but it's not that bad. LGTM. I only fear that the cols_so_far thing is easy to break, and the breakage will be easy to miss. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Jul-02, Tom Lane wrote:
>> * The persistence description values ought to be translatable, as
>> is the usual practice in describe.c. This is slightly painful
>> because it requires tweaking the translate_columns[] values in a
>> non-constant way, but it's not that bad.
> LGTM. I only fear that the cols_so_far thing is easy to break, and the
> breakage will be easy to miss.
Yeah, but that's pretty true of all the translatability stuff in
describe.c. I wonder if there's any way to set up tests for that.
The fact that the .po files lag so far behind the source code seems
like an impediment --- even if we made a test case that presumed
--enable-nls and tried to exercise this, the lack of translations
for the new words would get in the way for a long while.
regards, tom lane
> On 2 Jul 2019, at 22:16, Tom Lane <tgl@sss.pgh.pa.us> wrote: > even if we made a test case that presumed > --enable-nls and tried to exercise this, the lack of translations > for the new words would get in the way for a long while. For testing though, couldn’t we have an autogenerated .po which has a unique and predictable dummy value translation for every string (the string backwards or something), which can be used for testing? This is all hand-wavy since I haven’t tried actually doing it, but it seems a better option than waiting for .po files to be available. Or am I missing the point of the value of the discussed test? cheers ./daniel
On 2019-Jul-02, Daniel Gustafsson wrote: > > On 2 Jul 2019, at 22:16, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > even if we made a test case that presumed > > --enable-nls and tried to exercise this, the lack of translations > > for the new words would get in the way for a long while. > > For testing though, couldn’t we have an autogenerated .po which has a unique > and predictable dummy value translation for every string (the string backwards > or something), which can be used for testing? This is all hand-wavy since I > haven’t tried actually doing it, but it seems a better option than waiting for > .po files to be available. Or am I missing the point of the value of the > discussed test? Hmm, no, I think that's precisely it, and that sounds like a pretty good starter idea ... but I wouldn't want to be the one to have to set this up -- it seems pretty laborious. Anyway I'm not objecting to the patch -- I agree that we're already not testing translatability and that this patch shouldn't be forced to start doing it. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> On 2 Jul 2019, at 22:35, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Anyway I'm not objecting to the patch -- I agree that we're already not > testing translatability and that this patch shouldn't be forced to start > doing it. I forgot to add that to my previous email, the patch as it stands in v8 looks good to me. I’ve missed having this on many occasions. cheers ./daniel
Daniel Gustafsson <daniel@yesql.se> writes:
>> On 2 Jul 2019, at 22:35, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>> Anyway I'm not objecting to the patch -- I agree that we're already not
>> testing translatability and that this patch shouldn't be forced to start
>> doing it.
> I forgot to add that to my previous email, the patch as it stands in v8 looks
> good to me. I’ve missed having this on many occasions.
OK, pushed.
For the record, I did verify that the translatability logic worked
by adding some bogus entries to psql/po/es.po and seeing that the
display changed to match.
regards, tom lane