Обсуждение: Useless "Replica Identity: NOTHING" noise from psql \d
In HEAD:
regression=# \d pg_depend Table "pg_catalog.pg_depend" Column | Type | Modifiers
-------------+---------+-----------classid | oid | not nullobjid | oid | not nullobjsubid |
integer| not nullrefclassid | oid | not nullrefobjid | oid | not nullrefobjsubid | integer | not
nulldeptype | "char" | not null
Indexes: "pg_depend_depender_index" btree (classid, objid, objsubid) "pg_depend_reference_index" btree (refclassid,
refobjid,refobjsubid)
Replica Identity: NOTHING
Where did that last line come from, and who thinks it's so important
that it should appear by default? It seems absolutely content-free
even if I were using whatever feature it refers to, since it is
(I presume) the default state.
Please either suppress this entirely or at least condition it on \d+.
regards, tom lane
On 2013-12-14 11:27:53 -0500, Tom Lane wrote: > In HEAD: > > regression=# \d pg_depend > Table "pg_catalog.pg_depend" > Column | Type | Modifiers > -------------+---------+----------- > classid | oid | not null > objid | oid | not null > objsubid | integer | not null > refclassid | oid | not null > refobjid | oid | not null > refobjsubid | integer | not null > deptype | "char" | not null > Indexes: > "pg_depend_depender_index" btree (classid, objid, objsubid) > "pg_depend_reference_index" btree (refclassid, refobjid, refobjsubid) > Replica Identity: NOTHING > > Where did that last line come from, and who thinks it's so important > that it should appear by default? It seems absolutely content-free > even if I were using whatever feature it refers to, since it is > (I presume) the default state. Hm. Yes, that's slightly inellegant. It's shown because it's not actually the normal default normal tables. Just for system tables. Maybe we should just set it to default (in pg_class) for system tables as well, and just change it in the relcache. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 2013-12-14 17:43:36 +0100, Andres Freund wrote: > On 2013-12-14 11:27:53 -0500, Tom Lane wrote: > > In HEAD: > > > > regression=# \d pg_depend > > Table "pg_catalog.pg_depend" > > Column | Type | Modifiers > > -------------+---------+----------- > > classid | oid | not null > > objid | oid | not null > > objsubid | integer | not null > > refclassid | oid | not null > > refobjid | oid | not null > > refobjsubid | integer | not null > > deptype | "char" | not null > > Indexes: > > "pg_depend_depender_index" btree (classid, objid, objsubid) > > "pg_depend_reference_index" btree (refclassid, refobjid, refobjsubid) > > Replica Identity: NOTHING > > > > Where did that last line come from, and who thinks it's so important > > that it should appear by default? It seems absolutely content-free > > even if I were using whatever feature it refers to, since it is > > (I presume) the default state. > > Hm. Yes, that's slightly inellegant. It's shown because it's not > actually the normal default normal tables. Just for system tables. Maybe > we should just set it to default (in pg_class) for system tables as > well, and just change it in the relcache. Hm. I don't like that choice much after thinking for a bit. Seems to make querying the catalog unneccessarily complex. How about making it conditional on the table's namespace instead? That will do the wrong thing if somebody moves a table to pg_catalog and configures a replica identity, but I think we can live with that, given how many other things work strangely around that. Patch attached. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Вложения
On Mon, Dec 16, 2013 at 7:25 AM, Andres Freund <andres@2ndquadrant.com> wrote: > On 2013-12-14 17:43:36 +0100, Andres Freund wrote: >> On 2013-12-14 11:27:53 -0500, Tom Lane wrote: >> > In HEAD: >> > >> > regression=# \d pg_depend >> > Table "pg_catalog.pg_depend" >> > Column | Type | Modifiers >> > -------------+---------+----------- >> > classid | oid | not null >> > objid | oid | not null >> > objsubid | integer | not null >> > refclassid | oid | not null >> > refobjid | oid | not null >> > refobjsubid | integer | not null >> > deptype | "char" | not null >> > Indexes: >> > "pg_depend_depender_index" btree (classid, objid, objsubid) >> > "pg_depend_reference_index" btree (refclassid, refobjid, refobjsubid) >> > Replica Identity: NOTHING >> > >> > Where did that last line come from, and who thinks it's so important >> > that it should appear by default? It seems absolutely content-free >> > even if I were using whatever feature it refers to, since it is >> > (I presume) the default state. >> >> Hm. Yes, that's slightly inellegant. It's shown because it's not >> actually the normal default normal tables. Just for system tables. Maybe >> we should just set it to default (in pg_class) for system tables as >> well, and just change it in the relcache. > > Hm. I don't like that choice much after thinking for a bit. Seems to > make querying the catalog unneccessarily complex. > How about making it conditional on the table's namespace instead? That > will do the wrong thing if somebody moves a table to pg_catalog and > configures a replica identity, but I think we can live with that, given > how many other things work strangely around that. > > Patch attached. I vote for showing it only with +, but regardless of whether the value matches the expected default. I'd keep the relkind test, though, because I think I noticed that it currently shows up for indexes, which is dumb. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Dec 17, 2013 at 09:37:09AM -0500, Robert Haas wrote:
> > Patch attached.
>
> I vote for showing it only with +, but regardless of whether the value
> matches the expected default. I'd keep the relkind test, though,
> because I think I noticed that it currently shows up for indexes,
> which is dumb.
Is this the patch you had in mind? I kept the pg_catalog filter. Do we
want to always show the replica identity line for \d+?
test=> \d+ test
Table "public.test"
Column | Type | Modifiers | Storage | Stats target | Description
--------+---------+-----------+---------+--------------+-------------
x | integer | | plain | |
Replica Identity: full
Has OIDs: no
I used lower-case for the value, rather than all-caps.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
Вложения
Bruce Momjian <bruce@momjian.us> writes:
> Is this the patch you had in mind? I kept the pg_catalog filter. Do we
> want to always show the replica identity line for \d+?
Doesn't seem like a great idea to remove the filter tests for replident
values and then not fix the display code to cope with those values.
I think this display code is well south of minimal acceptability anyhow:
if the column contains anything other than what it expects, it will print
a lie, meaning it's entirely not future-proof. I'd suggest a switch()
that prints "???" in the default: case.
regards, tom lane
On Sun, Mar 23, 2014 at 11:49:37AM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Is this the patch you had in mind? I kept the pg_catalog filter. Do we > > want to always show the replica identity line for \d+? > > Doesn't seem like a great idea to remove the filter tests for replident > values and then not fix the display code to cope with those values. > > I think this display code is well south of minimal acceptability anyhow: > if the column contains anything other than what it expects, it will print > a lie, meaning it's entirely not future-proof. I'd suggest a switch() > that prints "???" in the default: case. Oh, good points. I have updated the attached patch. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Вложения
On 2014-03-22 23:47:57 -0400, Bruce Momjian wrote: > test=> \d+ test > Table "public.test" > Column | Type | Modifiers | Storage | Stats target | Description > --------+---------+-----------+---------+--------------+------------- > x | integer | | plain | | > Replica Identity: full > Has OIDs: no > > I used lower-case for the value, rather than all-caps. Why? CLUSTER, PRIMARY KEY, etc. are displayed all caps, and replica identity is similarly set via ALTER TABLE ... REPLICA IDENITY? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Mar 24, 2014 at 05:06:25PM +0100, Andres Freund wrote: > On 2014-03-22 23:47:57 -0400, Bruce Momjian wrote: > > test=> \d+ test > > Table "public.test" > > Column | Type | Modifiers | Storage | Stats target | Description > > --------+---------+-----------+---------+--------------+------------- > > x | integer | | plain | | > > Replica Identity: full > > Has OIDs: no > > > > I used lower-case for the value, rather than all-caps. > > Why? CLUSTER, PRIMARY KEY, etc. are displayed all caps, and replica > identity is similarly set via ALTER TABLE ... REPLICA IDENITY? Oh, good points; I had not considered PRIMARY KEY. Updated patch attached. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Вложения
Bruce Momjian wrote: > On Mon, Mar 24, 2014 at 05:06:25PM +0100, Andres Freund wrote: > > On 2014-03-22 23:47:57 -0400, Bruce Momjian wrote: > > > test=> \d+ test > > > Table "public.test" > > > Column | Type | Modifiers | Storage | Stats target | Description > > > --------+---------+-----------+---------+--------------+------------- > > > x | integer | | plain | | > > > Replica Identity: full > > > Has OIDs: no > > > > > > I used lower-case for the value, rather than all-caps. > > > > Why? CLUSTER, PRIMARY KEY, etc. are displayed all caps, and replica > > identity is similarly set via ALTER TABLE ... REPLICA IDENITY? > > Oh, good points; I had not considered PRIMARY KEY. Updated patch > attached. In the "INDEX" case, should the output mention specifically which index is being considered? -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Mar 24, 2014 at 01:35:20PM -0300, Alvaro Herrera wrote:
> Bruce Momjian wrote:
> > On Mon, Mar 24, 2014 at 05:06:25PM +0100, Andres Freund wrote:
> > > On 2014-03-22 23:47:57 -0400, Bruce Momjian wrote:
> > > > test=> \d+ test
> > > > Table "public.test"
> > > > Column | Type | Modifiers | Storage | Stats target | Description
> > > > --------+---------+-----------+---------+--------------+-------------
> > > > x | integer | | plain | |
> > > > Replica Identity: full
> > > > Has OIDs: no
> > > >
> > > > I used lower-case for the value, rather than all-caps.
> > >
> > > Why? CLUSTER, PRIMARY KEY, etc. are displayed all caps, and replica
> > > identity is similarly set via ALTER TABLE ... REPLICA IDENITY?
> >
> > Oh, good points; I had not considered PRIMARY KEY. Updated patch
> > attached.
>
> In the "INDEX" case, should the output mention specifically which index
> is being considered?
Ah, good idea. Updated patch attached. The output is now:
test=> \d+ test
Table "public.test"
Column | Type | Modifiers | Storage | Stats target | Description
--------+---------+-----------+---------+--------------+-------------
x | integer | not null | plain | |
Indexes:
"test_pkey" PRIMARY KEY, btree (x) REPLICA IDENTITY
"i_test2" btree (x)
--> Replica Identity: USING INDEX "test_pkey"
Has OIDs: no
However, now that I look at it, it seems redundant as REPLICA IDENTITY
is already marked on the actual index. Ideas?
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
Вложения
On Mon, Mar 24, 2014 at 09:07:07PM -0400, Bruce Momjian wrote: > > In the "INDEX" case, should the output mention specifically which index > > is being considered? > > Ah, good idea. Updated patch attached. The output is now: > > test=> \d+ test > Table "public.test" > Column | Type | Modifiers | Storage | Stats target | Description > --------+---------+-----------+---------+--------------+------------- > x | integer | not null | plain | | > Indexes: > "test_pkey" PRIMARY KEY, btree (x) REPLICA IDENTITY > "i_test2" btree (x) > --> Replica Identity: USING INDEX "test_pkey" > Has OIDs: no > > However, now that I look at it, it seems redundant as REPLICA IDENTITY > is already marked on the actual index. Ideas? Hearing nothing, I have gone back to the previous patch that just marks replica identity as USING INDEX; applied patch attached. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Вложения
Bruce Momjian wrote: > On Mon, Mar 24, 2014 at 09:07:07PM -0400, Bruce Momjian wrote: > > > In the "INDEX" case, should the output mention specifically which index > > > is being considered? > > > > Ah, good idea. Updated patch attached. The output is now: > > > > test=> \d+ test > > Table "public.test" > > Column | Type | Modifiers | Storage | Stats target | Description > > --------+---------+-----------+---------+--------------+------------- > > x | integer | not null | plain | | > > Indexes: > > "test_pkey" PRIMARY KEY, btree (x) REPLICA IDENTITY > > "i_test2" btree (x) > > --> Replica Identity: USING INDEX "test_pkey" > > Has OIDs: no > > > > However, now that I look at it, it seems redundant as REPLICA IDENTITY > > is already marked on the actual index. Ideas? > > Hearing nothing, I have gone back to the previous patch that just marks > replica identity as USING INDEX; applied patch attached. Not opposed to this, but it seems a bit strange; REPLICA IDENTITY is a property of the table, not of any individual index. I think we should lose the token in the "Indexes" section. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Wed, Mar 26, 2014 at 12:20:07PM -0300, Alvaro Herrera wrote:
> Bruce Momjian wrote:
> > On Mon, Mar 24, 2014 at 09:07:07PM -0400, Bruce Momjian wrote:
> > > > In the "INDEX" case, should the output mention specifically which index
> > > > is being considered?
> > >
> > > Ah, good idea. Updated patch attached. The output is now:
> > >
> > > test=> \d+ test
> > > Table "public.test"
> > > Column | Type | Modifiers | Storage | Stats target | Description
> > > --------+---------+-----------+---------+--------------+-------------
> > > x | integer | not null | plain | |
> > > Indexes:
> > > "test_pkey" PRIMARY KEY, btree (x) REPLICA IDENTITY
> > > "i_test2" btree (x)
> > > --> Replica Identity: USING INDEX "test_pkey"
> > > Has OIDs: no
> > >
> > > However, now that I look at it, it seems redundant as REPLICA IDENTITY
> > > is already marked on the actual index. Ideas?
> >
> > Hearing nothing, I have gone back to the previous patch that just marks
> > replica identity as USING INDEX; applied patch attached.
>
> Not opposed to this, but it seems a bit strange; REPLICA IDENTITY is a
> property of the table, not of any individual index. I think we should
> lose the token in the "Indexes" section.
That is an interesting idea. It would mean that \d table would not show
anything about replica identity, because right now it does:
test=> \d test Table "public.test" Column | Type | Modifiers--------+---------+----------- x | integer |
notnullIndexes: "test_pkey" PRIMARY KEY, btree (x) REPLICA IDENTITY
That seems logical. So under the new plan, \d would show:
test=> \d test Table "public.test" Column | Type | Modifiers--------+---------+----------- x | integer |
notnullIndexes: "test_pkey" PRIMARY KEY, btree (x)
and \d+ would show:
test=> \d+ test Table "public.test" Column | Type | Modifiers | Storage | Stats
target| Description --------+---------+-----------+---------+--------------+------------- x | integer | not
null | plain | | Indexes: "test_pkey" PRIMARY KEY, btree (x)Replica Identity: USING INDEX
"test_pkey" Has OIDs: no
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ Everyone has their own god. +
Bruce Momjian wrote: > On Wed, Mar 26, 2014 at 12:20:07PM -0300, Alvaro Herrera wrote: > > Not opposed to this, but it seems a bit strange; REPLICA IDENTITY is a > > property of the table, not of any individual index. I think we should > > lose the token in the "Indexes" section. > > That is an interesting idea. It would mean that \d table would not show > anything about replica identity, because right now it does: > > test=> \d test > Table "public.test" > Column | Type | Modifiers > --------+---------+----------- > x | integer | not null > Indexes: > "test_pkey" PRIMARY KEY, btree (x) REPLICA IDENTITY > > That seems logical. Hmm. It seems to me that to make this more compact we could keep the current token in the index line if it's INDEX, and not display the Replica Identity: line at all; and if it's something other than index and different from the default value, then print "Replica Identity" in both \d and \d+. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Wed, Mar 26, 2014 at 12:53:32PM -0300, Alvaro Herrera wrote:
> Bruce Momjian wrote:
> > On Wed, Mar 26, 2014 at 12:20:07PM -0300, Alvaro Herrera wrote:
>
> > > Not opposed to this, but it seems a bit strange; REPLICA IDENTITY is a
> > > property of the table, not of any individual index. I think we should
> > > lose the token in the "Indexes" section.
> >
> > That is an interesting idea. It would mean that \d table would not show
> > anything about replica identity, because right now it does:
> >
> > test=> \d test
> > Table "public.test"
> > Column | Type | Modifiers
> > --------+---------+-----------
> > x | integer | not null
> > Indexes:
> > "test_pkey" PRIMARY KEY, btree (x) REPLICA IDENTITY
> >
> > That seems logical.
>
> Hmm. It seems to me that to make this more compact we could keep the
> current token in the index line if it's INDEX, and not display the
> Replica Identity: line at all; and if it's something other than index
> and different from the default value, then print "Replica Identity" in
> both \d and \d+.
OK. Tom's original complaint was about showing the default state in \d:
http://www.postgresql.org/message-id/12303.1387038473@sss.pgh.pa.us
though that example was for an odd case where a system table didn't use
the default value.
The attached patch matches your suggestion. It is basically back to
what the code originally had, except it skips system tables, and shows
"???" for invalid values.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
Вложения
Re: Bruce Momjian 2014-03-26 <20140326161056.GA468@momjian.us>
> The attached patch matches your suggestion. It is basically back to
> what the code originally had, except it skips system tables, and shows
> "???" for invalid values.
Fwiw, "make check-world" is currently broken:
******** build/contrib/test_decoding/regression.diffs ********
*** /tmp/buildd/postgresql-9.4-9.4~20140327.0501/build/../contrib/test_decoding/expected/ddl.out Thu Mar 27 02:43:36
2014
--- /tmp/buildd/postgresql-9.4-9.4~20140327.0501/build/contrib/test_decoding/results/ddl.out Thu Mar 27 05:14:02
2014
***************
*** 345,350 ****
--- 345,351 ---- options | text[] | | extended |
| Indexes: "replication_metadata_pkey" PRIMARY KEY, btree (id)
+ Replica Identity: DEFAULT Has OIDs: no Options: user_catalog_table=true
***************
*** 360,365 ****
--- 361,367 ---- options | text[] | | extended |
| Indexes: "replication_metadata_pkey" PRIMARY KEY, btree (id)
+ Replica Identity: DEFAULT Has OIDs: no INSERT INTO replication_metadata(relation, options)
***************
*** 374,379 ****
--- 376,382 ---- options | text[] | | extended |
| Indexes: "replication_metadata_pkey" PRIMARY KEY, btree (id)
+ Replica Identity: DEFAULT Has OIDs: no Options: user_catalog_table=true
***************
*** 394,399 ****
--- 397,403 ---- rewritemeornot | integer | | plain
| | Indexes: "replication_metadata_pkey" PRIMARY KEY, btree (id)
+ Replica Identity: DEFAULT Has OIDs: no Options: user_catalog_table=false
Christoph
--
cb@df7cb.de | http://www.df7cb.de/
On Thu, Mar 27, 2014 at 10:02:20AM +0100, Christoph Berg wrote:
> Re: Bruce Momjian 2014-03-26 <20140326161056.GA468@momjian.us>
> > The attached patch matches your suggestion. It is basically back to
> > what the code originally had, except it skips system tables, and shows
> > "???" for invalid values.
>
> Fwiw, "make check-world" is currently broken:
Yes, the patch was partial to just show the code changes, to get
approval. Attached is the full patch.
I did some research of the regression database to see what was being
set:
SELECT relreplident, relkind, nspname, count(*)
FROM pg_class, pg_namespace
WHERE relkind IN ('m', 'r') AND
relnamespace = pg_namespace.oid AND
nspname != 'pg_catalog'
GROUP BY relreplident, nspname, relkind
ORDER BY 1, 2, 3;
relreplident | relkind | nspname | count
--------------+---------+--------------------+-------
d | m | mvschema | 1
d | m | public | 5
d | r | information_schema | 7
d | r | public | 205
d | r | testxmlschema | 3
It seems everything is default, which would not be displayed.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
Вложения
Re: Bruce Momjian 2014-03-27 <20140327131048.GA11694@momjian.us> > On Thu, Mar 27, 2014 at 10:02:20AM +0100, Christoph Berg wrote: > > Re: Bruce Momjian 2014-03-26 <20140326161056.GA468@momjian.us> > > > The attached patch matches your suggestion. It is basically back to > > > what the code originally had, except it skips system tables, and shows > > > "???" for invalid values. > > > > Fwiw, "make check-world" is currently broken: > > Yes, the patch was partial to just show the code changes, to get > approval. Attached is the full patch. I meant to say what's actually in git HEAD at the moment is broken. The regression.diffs are without an extra patch. This prevents building 9.4devel packages for apt.postgresql.org now. Mit freundlichen Grüßen, Christoph Berg -- Senior Berater, Tel.: +49 (0)21 61 / 46 43-187 credativ GmbH, HRB Mönchengladbach 12080, USt-ID-Nummer: DE204566209 Hohenzollernstr. 133, 41061 Mönchengladbach Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer pgp fingerprint: 5C48 FE61 57F4 9179 5970 87C6 4C5A 6BAB 12D2 A7AE
On Thu, Mar 27, 2014 at 02:41:40PM +0100, Christoph Berg wrote: > Re: Bruce Momjian 2014-03-27 <20140327131048.GA11694@momjian.us> > > On Thu, Mar 27, 2014 at 10:02:20AM +0100, Christoph Berg wrote: > > > Re: Bruce Momjian 2014-03-26 <20140326161056.GA468@momjian.us> > > > > The attached patch matches your suggestion. It is basically back to > > > > what the code originally had, except it skips system tables, and shows > > > > "???" for invalid values. > > > > > > Fwiw, "make check-world" is currently broken: > > > > Yes, the patch was partial to just show the code changes, to get > > approval. Attached is the full patch. > > I meant to say what's actually in git HEAD at the moment is broken. > The regression.diffs are without an extra patch. This prevents > building 9.4devel packages for apt.postgresql.org now. Uh, I thought that might be what you were saying, but I am not seeing any failures here. I don't see any platform-specific regression files in the files I modified. Can you show me the failures? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce Momjian <bruce@momjian.us> writes:
> On Thu, Mar 27, 2014 at 02:41:40PM +0100, Christoph Berg wrote:
>> I meant to say what's actually in git HEAD at the moment is broken.
> Uh, I thought that might be what you were saying, but I am not seeing
> any failures here.
The buildfarm isn't complaining, either. Is there some part of "make
check-world" that isn't exercised by the buildfarm?
regards, tom lane
On Thu, Mar 27, 2014 at 10:13:36AM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > On Thu, Mar 27, 2014 at 02:41:40PM +0100, Christoph Berg wrote: > >> I meant to say what's actually in git HEAD at the moment is broken. > > > Uh, I thought that might be what you were saying, but I am not seeing > > any failures here. > > The buildfarm isn't complaining, either. Is there some part of "make > check-world" that isn't exercised by the buildfarm? I see it now in contrib/test_decoding; fixing. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On 03/27/2014 10:13 AM, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: >> On Thu, Mar 27, 2014 at 02:41:40PM +0100, Christoph Berg wrote: >>> I meant to say what's actually in git HEAD at the moment is broken. >> Uh, I thought that might be what you were saying, but I am not seeing >> any failures here. > The buildfarm isn't complaining, either. Is there some part of "make > check-world" that isn't exercised by the buildfarm? > > No, unless you're building with some options the buildfarm doesn't exercise. (Well, the buildfarm does installcheck rather than check for contrib, but that should not matter.) cheers andrew
On Thu, Mar 27, 2014 at 10:30:59AM -0400, Bruce Momjian wrote: > On Thu, Mar 27, 2014 at 10:13:36AM -0400, Tom Lane wrote: > > Bruce Momjian <bruce@momjian.us> writes: > > > On Thu, Mar 27, 2014 at 02:41:40PM +0100, Christoph Berg wrote: > > >> I meant to say what's actually in git HEAD at the moment is broken. > > > > > Uh, I thought that might be what you were saying, but I am not seeing > > > any failures here. > > > > The buildfarm isn't complaining, either. Is there some part of "make > > check-world" that isn't exercised by the buildfarm? > > I see it now in contrib/test_decoding; fixing. OK, fixed. I have also updated my new patch to reflect those changes, attached. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Вложения
On 03/27/2014 10:31 AM, Andrew Dunstan wrote: > > On 03/27/2014 10:13 AM, Tom Lane wrote: >> Bruce Momjian <bruce@momjian.us> writes: >>> On Thu, Mar 27, 2014 at 02:41:40PM +0100, Christoph Berg wrote: >>>> I meant to say what's actually in git HEAD at the moment is broken. >>> Uh, I thought that might be what you were saying, but I am not seeing >>> any failures here. >> The buildfarm isn't complaining, either. Is there some part of "make >> check-world" that isn't exercised by the buildfarm? >> >> > > No, unless you're building with some options the buildfarm doesn't > exercise. (Well, the buildfarm does installcheck rather than check for > contrib, but that should not matter.) > > And I see it does. I missed that, and I don't think anyone mentioned it to me :-( I guess we'd better add a make-contrib-check step to the buildfarm. I was just prepping a release, so I'll delay that while I add this. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> I guess we'd better add a make-contrib-check step to the buildfarm. I
> was just prepping a release, so I'll delay that while I add this.
BTW, won't that obsolete the need for the separate check-pg_upgrade
step?
regards, tom lane
On 03/27/2014 11:27 AM, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> I guess we'd better add a make-contrib-check step to the buildfarm. I >> was just prepping a release, so I'll delay that while I add this. > BTW, won't that obsolete the need for the separate check-pg_upgrade > step? > > Yes, possibly. It helps if people bring to my attention changes in the build and test infrastructure, sometimes I miss developments, as I did here. cheers andrew
Re: Andrew Dunstan 2014-03-27 <53344465.6010401@dunslane.net> > > On 03/27/2014 11:27 AM, Tom Lane wrote: > >Andrew Dunstan <andrew@dunslane.net> writes: > >>I guess we'd better add a make-contrib-check step to the buildfarm. I > >>was just prepping a release, so I'll delay that while I add this. > >BTW, won't that obsolete the need for the separate check-pg_upgrade > >step? > > Yes, possibly. > > It helps if people bring to my attention changes in the build and test > infrastructure, sometimes I miss developments, as I did here. Why not "make check-world"? That should include everything, and doesn't need updating the scripts when something new gets included in PostgreSQL itself. The pg_upgrade test is included. Mit freundlichen Grüßen, Christoph Berg -- Senior Berater, Tel.: +49 (0)21 61 / 46 43-187 credativ GmbH, HRB Mönchengladbach 12080, USt-ID-Nummer: DE204566209 Hohenzollernstr. 133, 41061 Mönchengladbach Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer pgp fingerprint: 5C48 FE61 57F4 9179 5970 87C6 4C5A 6BAB 12D2 A7AE
Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > On Thu, Mar 27, 2014 at 02:41:40PM +0100, Christoph Berg wrote: > >> I meant to say what's actually in git HEAD at the moment is broken. > > > Uh, I thought that might be what you were saying, but I am not seeing > > any failures here. > > The buildfarm isn't complaining, either. Is there some part of "make > check-world" that isn't exercised by the buildfarm? I'd bet it's "make check" in contrib. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 03/27/2014 11:34 AM, Christoph Berg wrote: > Re: Andrew Dunstan 2014-03-27 <53344465.6010401@dunslane.net> >> On 03/27/2014 11:27 AM, Tom Lane wrote: >>> Andrew Dunstan <andrew@dunslane.net> writes: >>>> I guess we'd better add a make-contrib-check step to the buildfarm. I >>>> was just prepping a release, so I'll delay that while I add this. >>> BTW, won't that obsolete the need for the separate check-pg_upgrade >>> step? >> Yes, possibly. >> >> It helps if people bring to my attention changes in the build and test >> infrastructure, sometimes I miss developments, as I did here. > Why not "make check-world"? That should include everything, and > doesn't need updating the scripts when something new gets included in > PostgreSQL itself. The pg_upgrade test is included. > For several reasons. It's not simply a matter of running that command. You also have to bundle up the various log files. Also, if all we do is "check-world" and it fails that fact gives us relatively little information, whereas if it passes "make check" but fails "make -C contrib check" we'll have more information. So I'm not terribly excited about combining steps too much. cheers andrew
Andrew Dunstan wrote: > For several reasons. It's not simply a matter of running that > command. You also have to bundle up the various log files. Also, if > all we do is "check-world" and it fails that fact gives us > relatively little information, whereas if it passes "make check" but > fails "make -C contrib check" we'll have more information. So I'm > not terribly excited about combining steps too much. Note that "make check" in contrib is substantially slower than "make installcheck" --- it creates a temporary installation for each contrib module. If we make buildfarm run installcheck for all modules, that might be a problem for some animals. Would it be possible to have a target that runs "make installcheck" for most modules and "make check" only for those that need the separate installation? Maybe we can have a file listing modules that don't support installcheck, for instance, and then use $(filter) and $(filter-out) to produce appropriate "$(MAKE) -C" loops. Also, there's the vcregress.pl business. The way it essentially duplicates pg_upgrade/test.sh is rather messy; and now that test_decoding also needs similar treatment, it's not looking so good. Should we consider redoing that stuff in a way that allows both MSVC and make-based systems to run those tests? -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 2014-03-27 11:12:41 -0400, Andrew Dunstan wrote: > >No, unless you're building with some options the buildfarm doesn't > >exercise. (Well, the buildfarm does installcheck rather than check for > >contrib, but that should not matter.) > And I see it does. I missed that, and I don't think anyone mentioned it to > me :-( I tried to ping you about it :) The background is that we can't run against a installed installation because we require nonstandard PGC_POSTMASTER settings (wal_level, max_replication_slots). I hope there will be more tests that depend on wal_level sometime soon... > I guess we'd better add a make-contrib-check step to the buildfarm. I was > just prepping a release, so I'll delay that while I add this. Yes, I think that's a good idea. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Note that "make check" in contrib is substantially slower than "make
> installcheck" --- it creates a temporary installation for each contrib
> module. If we make buildfarm run installcheck for all modules, that
> might be a problem for some animals. Would it be possible to have a
> target that runs "make installcheck" for most modules and "make check"
> only for those that need the separate installation? Maybe we can have a
> file listing modules that don't support installcheck, for instance, and
> then use $(filter) and $(filter-out) to produce appropriate "$(MAKE) -C"
> loops.
This seems like a good idea to me; the slower animals will be putting lots
of cycles into pretty-much-useless "make check" builds if we don't.
Rather than a separate file, though, I think a distinct target in
contrib/Makefile would be the best mechanism for keeping the list of
modules that lack installcheck support. Perhaps "make special-check"
or some name along that line?
> Also, there's the vcregress.pl business. The way it essentially
> duplicates pg_upgrade/test.sh is rather messy; and now that
> test_decoding also needs similar treatment, it's not looking so good.
> Should we consider redoing that stuff in a way that allows both MSVC and
> make-based systems to run those tests?
Agreed, but I'm not volunteering to fix that one ;-)
regards, tom lane
On Thu, Mar 27, 2014 at 12:56:02PM -0300, Alvaro Herrera wrote: > Also, there's the vcregress.pl business. The way it essentially > duplicates pg_upgrade/test.sh is rather messy; and now that > test_decoding also needs similar treatment, it's not looking so good. > Should we consider redoing that stuff in a way that allows both MSVC and > make-based systems to run those tests? +1 Incidentally, I've seen the following row-order difference for test_decoding. (I assumed the buildfarm would notice that quickly enough, but this thread has corrected that assumption.) Barring objections, I'll make it "ORDER BY 1,2". --- /home/nmisch/src/pg/postgresql/contrib/test_decoding/expected/ddl.out 2014-03-23 07:32:25.718189175 +0000 +++ /home/nmisch/src/pg/postgresql/contrib/test_decoding/results/ddl.out 2014-03-27 17:40:33.079815557 +0000 @@ -199,8 +199,8 @@ORDER BY 1; count | min | max -------+-------------------------------------------------+------------------------------------------------------------------------ - 1 | COMMIT | COMMIT 1 | BEGIN | BEGIN + 1 | COMMIT | COMMIT 20467 | table public.tr_etoomuch: DELETE: id[integer]:1| table public.tr_etoomuch: UPDATE: id[integer]:9999 data[integer]:-9999(3 rows) -- Noah Misch EnterpriseDB http://www.enterprisedb.com
On 03/27/2014 11:56 AM, Alvaro Herrera wrote: > Also, there's the vcregress.pl business. The way it essentially > duplicates pg_upgrade/test.sh is rather messy; and now that > test_decoding also needs similar treatment, it's not looking so good. > Should we consider redoing that stuff in a way that allows both MSVC and > make-based systems to run those tests? > Well, to start with people need to get out of the habit of writing tests in shell script. I know it's hard, we've only had the Windows port for 9 years or so, so it takes a bit of getting used to ... cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 03/27/2014 11:56 AM, Alvaro Herrera wrote:
>> Also, there's the vcregress.pl business. The way it essentially
>> duplicates pg_upgrade/test.sh is rather messy; and now that
>> test_decoding also needs similar treatment, it's not looking so good.
>> Should we consider redoing that stuff in a way that allows both MSVC and
>> make-based systems to run those tests?
> Well, to start with people need to get out of the habit of writing tests
> in shell script.
What alternative do you propose? We have a policy of not requiring Perl
to build/test, so don't suggest that.
I'm inclined to think the problem with test.sh is not so much the language
that it's in, as that it's single-purpose. Maybe it has to be given the
nature of the pg_upgrade tests, but we should look for some generality.
I'd be happy if we had shell-based infrastructure on non-Windows and a
separate Perl equivalent for Windows, as long as we didn't have to start
from scratch for each special-configuration test scenario.
regards, tom lane
On 03/27/2014 04:31 PM, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> On 03/27/2014 11:56 AM, Alvaro Herrera wrote: >>> Also, there's the vcregress.pl business. The way it essentially >>> duplicates pg_upgrade/test.sh is rather messy; and now that >>> test_decoding also needs similar treatment, it's not looking so good. >>> Should we consider redoing that stuff in a way that allows both MSVC and >>> make-based systems to run those tests? >> Well, to start with people need to get out of the habit of writing tests >> in shell script. > What alternative do you propose? We have a policy of not requiring Perl > to build/test, so don't suggest that. > > I'm inclined to think the problem with test.sh is not so much the language > that it's in, as that it's single-purpose. Maybe it has to be given the > nature of the pg_upgrade tests, but we should look for some generality. > I'd be happy if we had shell-based infrastructure on non-Windows and a > separate Perl equivalent for Windows, as long as we didn't have to start > from scratch for each special-configuration test scenario. > > If you can create it so it's somehow config driven, we can surely replicate the engine in Perl. But I'm not going to hold my breath waiting. cheers andrew
On 2014-03-27 13:52:19 -0400, Noah Misch wrote:
> On Thu, Mar 27, 2014 at 12:56:02PM -0300, Alvaro Herrera wrote:
> > Also, there's the vcregress.pl business. The way it essentially
> > duplicates pg_upgrade/test.sh is rather messy; and now that
> > test_decoding also needs similar treatment, it's not looking so good.
> > Should we consider redoing that stuff in a way that allows both MSVC and
> > make-based systems to run those tests?
>
> +1
I'd like that as well, but I don't really see how.
> Incidentally, I've seen the following row-order difference for test_decoding.
> (I assumed the buildfarm would notice that quickly enough, but this thread has
> corrected that assumption.) Barring objections, I'll make it "ORDER BY 1,2".
>
> --- /home/nmisch/src/pg/postgresql/contrib/test_decoding/expected/ddl.out 2014-03-23 07:32:25.718189175 +0000
> +++ /home/nmisch/src/pg/postgresql/contrib/test_decoding/results/ddl.out 2014-03-27 17:40:33.079815557 +0000
> @@ -199,8 +199,8 @@
> ORDER BY 1;
> count | min | max
>
-------+-------------------------------------------------+------------------------------------------------------------------------
> - 1 | COMMIT | COMMIT
> 1 | BEGIN | BEGIN
> + 1 | COMMIT | COMMIT
> 20467 | table public.tr_etoomuch: DELETE: id[integer]:1 | table public.tr_etoomuch: UPDATE: id[integer]:9999
data[integer]:-9999
> (3 rows)
That sounds like a good plan. Thanks!
Greetings,
Andres Freund
-- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services
On 2014-03-27 12:56:02 -0300, Alvaro Herrera wrote: > Also, there's the vcregress.pl business. The way it essentially > duplicates pg_upgrade/test.sh is rather messy; and now that > test_decoding also needs similar treatment, it's not looking so good. > Should we consider redoing that stuff in a way that allows both MSVC and > make-based systems to run those tests? I really hope test_decoding needs less scaffolding than this? There's much less going on in its test, right? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 03/27/2014 05:15 PM, Andres Freund wrote: > On 2014-03-27 12:56:02 -0300, Alvaro Herrera wrote: >> Also, there's the vcregress.pl business. The way it essentially >> duplicates pg_upgrade/test.sh is rather messy; and now that >> test_decoding also needs similar treatment, it's not looking so good. >> Should we consider redoing that stuff in a way that allows both MSVC and >> make-based systems to run those tests? > I really hope test_decoding needs less scaffolding than this? There's > much less going on in its test, right? Yeah. What I have done is create a quite small buildfarm module to run these tests. See <https://github.com/PGBuildFarm/client-code/commit/69c92f53bbe3748c13fa29aee0c39c8dd7210f1e> It can be added to an existing buildfarm client installation and enabled by adding TestDecoding to the list of modules in the buildfarm config file. It will be included in the next release and enabled in that release's sample config file. See an example run at <http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=crake&dt=2014-03-27%2023%3A40%3A15&stg=test-decoding-check>. Larger questions can wait, but this makes a start on the immediate issue. cheers andrew
On Thu, Mar 27, 2014 at 11:02:55AM -0400, Bruce Momjian wrote: > On Thu, Mar 27, 2014 at 10:30:59AM -0400, Bruce Momjian wrote: > > On Thu, Mar 27, 2014 at 10:13:36AM -0400, Tom Lane wrote: > > > Bruce Momjian <bruce@momjian.us> writes: > > > > On Thu, Mar 27, 2014 at 02:41:40PM +0100, Christoph Berg wrote: > > > >> I meant to say what's actually in git HEAD at the moment is broken. > > > > > > > Uh, I thought that might be what you were saying, but I am not seeing > > > > any failures here. > > > > > > The buildfarm isn't complaining, either. Is there some part of "make > > > check-world" that isn't exercised by the buildfarm? > > > > I see it now in contrib/test_decoding; fixing. > > OK, fixed. I have also updated my new patch to reflect those changes, > attached. Applied. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On 03/27/2014 01:09 PM, Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: >> Note that "make check" in contrib is substantially slower than "make >> installcheck" --- it creates a temporary installation for each contrib >> module. If we make buildfarm run installcheck for all modules, that >> might be a problem for some animals. Would it be possible to have a >> target that runs "make installcheck" for most modules and "make check" >> only for those that need the separate installation? Maybe we can have a >> file listing modules that don't support installcheck, for instance, and >> then use $(filter) and $(filter-out) to produce appropriate "$(MAKE) -C" >> loops. > This seems like a good idea to me; the slower animals will be putting lots > of cycles into pretty-much-useless "make check" builds if we don't. > > Rather than a separate file, though, I think a distinct target in > contrib/Makefile would be the best mechanism for keeping the list of > modules that lack installcheck support. Perhaps "make special-check" > or some name along that line? > >> Also, there's the vcregress.pl business. The way it essentially >> duplicates pg_upgrade/test.sh is rather messy; and now that >> test_decoding also needs similar treatment, it's not looking so good. >> Should we consider redoing that stuff in a way that allows both MSVC and >> make-based systems to run those tests? > Agreed, but I'm not volunteering to fix that one ;-) > > I've been kind of hoping that someone would step up on both these items, but the trail seems to have gone cold. I'm going to put out the new buildfarm release with the new module to run test_decoding check. But of course It won't have MSVC support. These can go on my long TOO list unless someone else gets there first. cheers andrew
Re: Useless "Replica Identity: NOTHING" noise from psql \d
От
andres@anarazel.de (Andres Freund)
Дата:
On 2014-04-03 14:49:54 -0400, Andrew Dunstan wrote: > I've been kind of hoping that someone would step up on both these items, but > the trail seems to have gone cold. > > I'm going to put out the new buildfarm release with the new module to run > test_decoding check. But of course It won't have MSVC support. > > These can go on my long TOO list unless someone else gets there first. So, I was thinking on how we can improve this situation. There's basically one bigger remaining problem besides make check vs. make installcheck: Currently contrib modules can't easily run isolationtester in a general fashion. That's why test_decoding's Makefile has to rig that all itself. How about simply improving the contrib support to recognize tests in ISOLATION_REGRESS in addition to the current REGRESS? Then we can "simply" train vcregress.pl to pick them up generally, without special support for test_decoding. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Sat, Apr 5, 2014 at 7:12 AM, Andres Freund <andres@anarazel.de> wrote: > On 2014-04-03 14:49:54 -0400, Andrew Dunstan wrote: >> I've been kind of hoping that someone would step up on both these items, but >> the trail seems to have gone cold. >> >> I'm going to put out the new buildfarm release with the new module to run >> test_decoding check. But of course It won't have MSVC support. >> >> These can go on my long TOO list unless someone else gets there first. > > So, I was thinking on how we can improve this situation. There's > basically one bigger remaining problem besides make check vs. make > installcheck: > Currently contrib modules can't easily run isolationtester in a general > fashion. That's why test_decoding's Makefile has to rig that all > itself. How about simply improving the contrib support to recognize > tests in ISOLATION_REGRESS in addition to the current REGRESS? Then we > can "simply" train vcregress.pl to pick them up generally, without > special support for test_decoding. IMHO, that's a fine idea. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company