Re: [HACKERS] inherit support for foreign tables
Hello,
analyze.c: In function ‘acquire_inherited_sample_rows’:
analyze.c:1461: warning: unused variable ‘saved_rel’
I've fixed this in the latest version (v8) of the patch.
Mmm. sorry. I missed v8 patch. Then I had a look on that and have
a question.
You've added a check for relkind of baserel of the foreign path
to be reparameterized. If this should be an assertion - not a
conditional branch -, it would be better to put the assertion in
create_foreignscan_path instead of there.
=
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -1723,6 +1723,7 @@ create_foreignscan_path(PlannerInfo *root, RelOptInfo
*rel,
List *fdw_private)
{
ForeignPath *pathnode = makeNode(ForeignPath);
+ Assert(rel-rtekind == RTE_RELATION);
pathnode-path.pathtype = T_ForeignScan;
pathnode-path.parent = rel;
=
And for file-fdw, you made a change to re-create foreignscan node
instead of the previous copy-and-modify. Is the reason you did it
that you considered the cost of 're-checking whether to
selectively perform binary conversion' is low enough? Or other
reasons?
The reason is that we get the result of the recheck from
path-fdw_private. Sorry, I'd forgotten it. So, I modified the code to
simply call create_foreignscan_path().
Anyway you new code seems closer to the basics and the gain by
the previous optimization don't seem to be significant..
Finally, although I insist the necessity of the warning for child
foreign tables on alter table, if you belive it to be put off,
I'll compromise by putting a note to CF-app that last judgement
is left to committer.
OK So, if there are no objections of other, I'll mark this patch as
ready for committer and do that.
Thank you.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] datistemplate of pg_database does not behave as per description in documentation
As per the documentation, datistemplate of pg_database is used in following way: datistemplate Bool If true then this database can be used in the TEMPLATE clause of CREATE DATABASE to create a new database as a clone of this one But current code does not behave in this manner. Even if dbistemplate of database is false, still it allows to be used as template database. postgres=# select datname, datistemplate from pg_database; datname | datistemplate ---+--- template1 | t template0 | t postgres | f (3 rows) postgres=# create database tempdb template postgres; ---Actually this should fail. CREATE DATABASE Though I am not sure if we have to modify source code to align the behavior with documentation or we need to change the documentation itself. To me it looks like code change will be better, so I am attaching the current patch with source code change. After modification, result will be as follows: postgres=# create database newtempdb template postgres; ERROR: DB name postgres given as template is not a template database Please provide your feedback. Thanks and Regards, Kumar Rajeev Rastogi datistemplate_issue.patch Description: datistemplate_issue.patch -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] datistemplate of pg_database does not behave as per description in documentation
On Thu, Mar 27, 2014 at 9:12 AM, Rajeev rastogi rajeev.rast...@huawei.comwrote: As per the documentation, datistemplate of pg_database is used in following way: datistemplate Bool If true then this database can be used in the TEMPLATE clause of CREATE DATABASE to create a new database as a clone of this one But current code does not behave in this manner. Even if dbistemplate of database is false, still it allows to be used as template database. postgres=# select datname, datistemplate from pg_database; datname | datistemplate ---+--- template1 | t template0 | t *postgres | f* (3 rows) *postgres=# create database tempdb template postgres; ---Actually this should fail.* *CREATE DATABASE* Though I am not sure if we have to modify source code to align the behavior with documentation or we need to change the documentation itself. To me it looks like code change will be better, so I am attaching the current patch with source code change. After modification, result will be as follows: *postgres=# create database newtempdb template postgres;* *ERROR: DB name postgres given as template is not a template database* Please provide your feedback. AFAICT, the *only* thing datistemplate is used is to set parameters in autovacuum. So clearly we should do something. Changing the code that way carries the risk of breaking applications (or at least DBA scripts) for no apparent reason. I think it's better to document it. However, that also raises a third option. We could just drop the idea if datistemplate completely, and remove the column. Since clearly it's not actually doing anything, and people seem to have been happy with that for a while, why do we need it in the first place? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] inherit support for foreign tables
Hi Horiguchi-san,
(2014/03/27 17:09), Kyotaro HORIGUCHI wrote:
analyze.c: In function ‘acquire_inherited_sample_rows’:
analyze.c:1461: warning: unused variable ‘saved_rel’
I've fixed this in the latest version (v8) of the patch.
Mmm. sorry. I missed v8 patch. Then I had a look on that and have
a question.
Thank you for the review!
You've added a check for relkind of baserel of the foreign path
to be reparameterized. If this should be an assertion - not a
conditional branch -, it would be better to put the assertion in
create_foreignscan_path instead of there.
=
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -1723,6 +1723,7 @@ create_foreignscan_path(PlannerInfo *root, RelOptInfo
*rel,
List *fdw_private)
{
ForeignPath *pathnode = makeNode(ForeignPath);
+ Assert(rel-rtekind == RTE_RELATION);
pathnode-path.pathtype = T_ForeignScan;
pathnode-path.parent = rel;
=
Maybe I'm missing the point, but I don't think it'd be better to put the
assertion in create_foreignscan_path(). And I think it'd be the caller'
responsiblity to ensure that equality, as any other pathnode creation
routine for a baserel in pathnode.c assumes that equality.
And for file-fdw, you made a change to re-create foreignscan node
instead of the previous copy-and-modify. Is the reason you did it
that you considered the cost of 're-checking whether to
selectively perform binary conversion' is low enough? Or other
reasons?
The reason is that we get the result of the recheck from
path-fdw_private. Sorry, I'd forgotten it. So, I modified the code to
simply call create_foreignscan_path().
Anyway you new code seems closer to the basics and the gain by
the previous optimization don't seem to be significant..
Yeah, that's true. I have to admit that the previous optimization is
nonsense.
Thanks,
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Few new warnings have been introduced in windows build (jsonb_util.c)
On 03/27/2014 06:35 AM, Amit Kapila wrote: Few new warnings have been introduced in windows build. 1e:\workspace\postgresql\master\postgresql\src\backend\utils\adt\jsonb_util.c(802): warning C4715: 'JsonbIteratorNext' : not all control paths return a value 1e:\workspace\postgresql\master\postgresql\src\backend\utils\adt\jsonb_util.c(1042): warning C4715: 'JsonbDeepContains' : not all control paths return a value 1e:\workspace\postgresql\master\postgresql\src\backend\utils\adt\jsonb_util.c(1175): warning C4715: 'compareJsonbScalarValue' : not all control paths return a value These are quite similar to what we have fixed some time back. Attached patch fixes these warnings. I am not sure about fix of warning in 'JsonbIteratorNext', as there is no invalid value for JSONB processing. Thanks, applied. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
Re: Bruce Momjian 2014-03-26 20140326161056.ga...@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 -- c...@df7cb.de | http://www.df7cb.de/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] datistemplate of pg_database does not behave as per description in documentation
* Magnus Hagander (mag...@hagander.net) wrote: However, that also raises a third option. We could just drop the idea if datistemplate completely, and remove the column. Since clearly it's not actually doing anything, and people seem to have been happy with that for a while, why do we need it in the first place? It's a bit of extra meta-data which can be nice to have (I know we use that field for our own purposes..). On the flip side, I've never liked that you have to update pg_database to change it, so perhaps we should get rid of it and remove that temptation. The other field we regularly update in pg_database is datallowconn... Would love to see that as an actual ALTER DATABASE command instead... Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] datistemplate of pg_database does not behave as per description in documentation
Magnus Hagander mag...@hagander.net writes:
On Thu, Mar 27, 2014 at 9:12 AM, Rajeev rastogi
rajeev.rast...@huawei.comwrote:
But current code does not behave in this manner. Even if dbistemplate of
database is false, still it allows to be used as template database.
AFAICT, the *only* thing datistemplate is used is to set parameters in
autovacuum.
Huh? The code comment is perfectly clear:
/*
* Permission check: to copy a DB that's not marked datistemplate, you
* must be superuser or the owner thereof.
*/
if (!src_istemplate)
{
if (!pg_database_ownercheck(src_dboid, GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg(permission denied to copy database \%s\,
dbtemplate)));
}
I agree we need to make the docs match the code, but changing behavior
that's been like that for ten or fifteen years isn't the answer.
(Changing code and failing to adjust the adjacent comment is even less
of an answer.)
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Something flaky in the relfilenode mapping infrastructure
Buildfarm member prairiedog thinks there's something unreliable about commit f01d1ae3a104019d6d68aeff85c4816a275130b3: http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prairiedogdt=2014-03-27%2008%3A12%3A11 == pgsql.13462/src/test/regress/regression.diffs === *** /Users/buildfarm/bf-data/HEAD/pgsql.13462/src/test/regress/expected/alter_table.out Thu Mar 27 04:12:40 2014 --- /Users/buildfarm/bf-data/HEAD/pgsql.13462/src/test/regress/results/alter_table.out Thu Mar 27 04:52:02 2014 *** *** 2333,2339 ) mapped; incorrectly_mapped | have_mappings +--- ! 0 | t (1 row) -- Checks on creating and manipulation of user defined relations in --- 2333,2339 ) mapped; incorrectly_mapped | have_mappings +--- ! 1 | t (1 row) -- Checks on creating and manipulation of user defined relations in == regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] datistemplate of pg_database does not behave as per description in documentation
On Mar 27, 2014 12:45 PM, Tom Lane t...@sss.pgh.pa.us wrote:
Magnus Hagander mag...@hagander.net writes:
On Thu, Mar 27, 2014 at 9:12 AM, Rajeev rastogi
rajeev.rast...@huawei.comwrote:
But current code does not behave in this manner. Even if dbistemplate
of
database is false, still it allows to be used as template database.
AFAICT, the *only* thing datistemplate is used is to set parameters in
autovacuum.
Huh? The code comment is perfectly clear:
/*
* Permission check: to copy a DB that's not marked datistemplate, you
* must be superuser or the owner thereof.
*/
if (!src_istemplate)
{
if (!pg_database_ownercheck(src_dboid, GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg(permission denied to copy database \%s\,
dbtemplate)));
}
I agree we need to make the docs match the code, but changing behavior
that's been like that for ten or fifteen years isn't the answer.
Hah, I hadn't done more than git grep for datistemplate :-) that's what I
get for taking shortcuts...
But yes, fixing the documentation is what I advocate as well.
(Changing code and failing to adjust the adjacent comment is even less
of an answer.)
I didn't even look at the suggested patch..
/Magnus
Re: [HACKERS] Missing pfree in logical_heap_rewrite_flush_mappings()
* Andres Freund (and...@2ndquadrant.com) wrote: On 2014-03-26 13:41:27 -0400, Stephen Frost wrote: Good catch. There's actually no need for explicitly using the context, we're in the appropriate one. The only other MemoryContextAlloc() caller in there should be converted to a palloc as well. Hrm..? I don't think that's right when it's called from end_heap_rewrite(). You're right. Apprently I shouldn't look at patches while watching a keynote ;) No problem- but that's also why I was thinking we should just wrap end_heap_rewrite() in a context as well, otherwise the next person who comes along to add a bit of code here may end up making the same mistake. Is there something you're concerned about there? Perhaps we should be switching to state-rs_cxt while in end_heap_rewrite() also though? I think just applying Aant's patch is fine. I have no problem *also* doing the pfree() to address the concern about the transient memory usage being more than we'd like. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
On Thu, Mar 27, 2014 at 10:02:20AM +0100, Christoph Berg wrote:
Re: Bruce Momjian 2014-03-26 20140326161056.ga...@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 br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
new file mode 100644
index 21bbdf8..d1447fe
*** a/src/bin/psql/describe.c
--- b/src/bin/psql/describe.c
*** describeOneTableDetails(const char *sche
*** 2345,2360
printTableAddFooter(cont, buf.data);
}
! if (verbose (tableinfo.relkind == 'r' || tableinfo.relkind == 'm')
strcmp(schemaname, pg_catalog) != 0)
{
const char *s = _(Replica Identity);
printfPQExpBuffer(buf, %s: %s,
s,
- tableinfo.relreplident == 'd' ? DEFAULT :
tableinfo.relreplident == 'f' ? FULL :
- tableinfo.relreplident == 'i' ? USING INDEX :
tableinfo.relreplident == 'n' ? NOTHING :
???);
--- 2345,2363
printTableAddFooter(cont, buf.data);
}
! if ((tableinfo.relkind == 'r' || tableinfo.relkind == 'm')
! /*
! * No need to display default values; we already display a
! * REPLICA IDENTITY marker on indexes.
! */
! tableinfo.relreplident != 'd' tableinfo.relreplident != 'i'
strcmp(schemaname, pg_catalog) != 0)
{
const char *s = _(Replica Identity);
printfPQExpBuffer(buf, %s: %s,
s,
tableinfo.relreplident == 'f' ? FULL :
tableinfo.relreplident == 'n' ? NOTHING :
???);
diff --git a/src/test/regress/expected/create_table_like.out b/src/test/regress/expected/create_table_like.out
new file mode 100644
index feb6c93..5f29b39
*** a/src/test/regress/expected/create_table_like.out
--- b/src/test/regress/expected/create_table_like.out
*** CREATE TABLE ctlt12_storage (LIKE ctlt1
*** 115,121
a | text | not null | main | |
b | text | | extended | |
c | text | | external | |
- Replica Identity: DEFAULT
Has OIDs: no
CREATE TABLE ctlt12_comments (LIKE ctlt1 INCLUDING COMMENTS, LIKE ctlt2 INCLUDING COMMENTS);
--- 115,120
*** CREATE TABLE ctlt12_comments (LIKE ctlt1
*** 126,132
a | text | not null | extended | | A
b | text | | extended | | B
c | text | | extended | | C
- Replica Identity: DEFAULT
Has OIDs: no
CREATE TABLE ctlt1_inh (LIKE ctlt1 INCLUDING CONSTRAINTS INCLUDING COMMENTS) INHERITS (ctlt1);
--- 125,130
*** NOTICE: merging constraint ctlt1_a_che
*** 142,148
Check constraints:
ctlt1_a_check CHECK (length(a) 2)
Inherits: ctlt1
- Replica Identity: DEFAULT
Has OIDs: no
SELECT description FROM pg_description, pg_constraint c WHERE classoid = 'pg_constraint'::regclass AND objoid = c.oid AND c.conrelid = 'ctlt1_inh'::regclass;
--- 140,145
*** Check constraints:
*** 165,171
ctlt3_a_check CHECK (length(a) 5)
Inherits: ctlt1,
ctlt3
- Replica Identity: DEFAULT
Has OIDs: no
CREATE TABLE ctlt13_like (LIKE ctlt3 INCLUDING CONSTRAINTS INCLUDING COMMENTS INCLUDING STORAGE) INHERITS (ctlt1);
--- 162,167
*** Check constraints:
*** 181,187
ctlt1_a_check CHECK (length(a) 2)
ctlt3_a_check CHECK (length(a) 5)
Inherits: ctlt1
- Replica Identity: DEFAULT
Has OIDs: no
SELECT description FROM pg_description, pg_constraint c WHERE classoid = 'pg_constraint'::regclass AND objoid = c.oid AND c.conrelid = 'ctlt13_like'::regclass;
--- 177,182
*** Indexes:
***
[HACKERS] psql \d+ and oid display
When we made OIDs optional, we added an oid status display to \d+: test= \d+ test Table public.test Column | Type | Modifiers | Storage | Stats target | Description +-+---+-+--+- x | integer | | plain | | -- Has OIDs: no Do we want to continue displaying that OID line, or make it optional for cases where the value doesn't match default_with_oids? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
Re: Bruce Momjian 2014-03-27 20140327131048.ga11...@momjian.us On Thu, Mar 27, 2014 at 10:02:20AM +0100, Christoph Berg wrote: Re: Bruce Momjian 2014-03-26 20140326161056.ga...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
On Thu, Mar 27, 2014 at 02:41:40PM +0100, Christoph Berg wrote: Re: Bruce Momjian 2014-03-27 20140327131048.ga11...@momjian.us On Thu, Mar 27, 2014 at 10:02:20AM +0100, Christoph Berg wrote: Re: Bruce Momjian 2014-03-26 20140326161056.ga...@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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
Bruce Momjian br...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
On 03/27/2014 10:13 AM, Tom Lane wrote: Bruce Momjian br...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
On Thu, Mar 27, 2014 at 10:13:36AM -0400, Tom Lane wrote: Bruce Momjian br...@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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
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 br...@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 br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
diff --git a/contrib/test_decoding/expected/ddl.out b/contrib/test_decoding/expected/ddl.out
new file mode 100644
index 4d25a28..0dff60e
*** a/contrib/test_decoding/expected/ddl.out
--- b/contrib/test_decoding/expected/ddl.out
*** WITH (user_catalog_table = true)
*** 345,351
options | text[] | | extended | |
Indexes:
replication_metadata_pkey PRIMARY KEY, btree (id)
- Replica Identity: DEFAULT
Has OIDs: no
Options: user_catalog_table=true
--- 345,350
*** ALTER TABLE replication_metadata RESET (
*** 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)
--- 360,365
*** ALTER TABLE replication_metadata SET (us
*** 376,382
options | text[] | | extended | |
Indexes:
replication_metadata_pkey PRIMARY KEY, btree (id)
- Replica Identity: DEFAULT
Has OIDs: no
Options: user_catalog_table=true
--- 374,379
*** ALTER TABLE replication_metadata SET (us
*** 397,403
rewritemeornot | integer | | plain| |
Indexes:
replication_metadata_pkey PRIMARY KEY, btree (id)
- Replica Identity: DEFAULT
Has OIDs: no
Options: user_catalog_table=false
--- 394,399
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
new file mode 100644
index 21bbdf8..d1447fe
*** a/src/bin/psql/describe.c
--- b/src/bin/psql/describe.c
*** describeOneTableDetails(const char *sche
*** 2345,2360
printTableAddFooter(cont, buf.data);
}
! if (verbose (tableinfo.relkind == 'r' || tableinfo.relkind == 'm')
strcmp(schemaname, pg_catalog) != 0)
{
const char *s = _(Replica Identity);
printfPQExpBuffer(buf, %s: %s,
s,
- tableinfo.relreplident == 'd' ? DEFAULT :
tableinfo.relreplident == 'f' ? FULL :
- tableinfo.relreplident == 'i' ? USING INDEX :
tableinfo.relreplident == 'n' ? NOTHING :
???);
--- 2345,2363
printTableAddFooter(cont, buf.data);
}
! if ((tableinfo.relkind == 'r' || tableinfo.relkind == 'm')
! /*
! * No need to display default values; we already display a
! * REPLICA IDENTITY marker on indexes.
! */
! tableinfo.relreplident != 'd' tableinfo.relreplident != 'i'
strcmp(schemaname, pg_catalog) != 0)
{
const char *s = _(Replica Identity);
printfPQExpBuffer(buf, %s: %s,
s,
tableinfo.relreplident == 'f' ? FULL :
tableinfo.relreplident == 'n' ? NOTHING :
???);
diff --git a/src/test/regress/expected/create_table_like.out b/src/test/regress/expected/create_table_like.out
new file mode 100644
index feb6c93..5f29b39
*** a/src/test/regress/expected/create_table_like.out
--- b/src/test/regress/expected/create_table_like.out
*** CREATE TABLE ctlt12_storage (LIKE ctlt1
*** 115,121
a | text | not null | main | |
b | text | | extended | |
c | text | | external | |
- Replica Identity: DEFAULT
Has OIDs: no
CREATE TABLE ctlt12_comments (LIKE ctlt1 INCLUDING COMMENTS, LIKE ctlt2 INCLUDING COMMENTS);
--- 115,120
*** CREATE TABLE ctlt12_comments (LIKE ctlt1
*** 126,132
a | text | not null | extended | | A
b | text | | extended | | B
c | text | | extended | | C
- Replica Identity: DEFAULT
Has OIDs: no
CREATE TABLE ctlt1_inh (LIKE ctlt1 INCLUDING CONSTRAINTS INCLUDING COMMENTS) INHERITS (ctlt1);
--- 125,130
*** NOTICE: merging constraint ctlt1_a_che
*** 142,148
Check
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
On 03/27/2014 10:31 AM, Andrew Dunstan wrote: On 03/27/2014 10:13 AM, Tom Lane wrote: Bruce Momjian br...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
Andrew Dunstan and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
On 03/27/2014 11:27 AM, Tom Lane wrote: Andrew Dunstan and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
Tom Lane wrote: Bruce Momjian br...@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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
Re: Andrew Dunstan 2014-03-27 53344465.6010...@dunslane.net On 03/27/2014 11:27 AM, Tom Lane wrote: Andrew Dunstan and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Negative Transition Aggregate Functions (WIP)
On 26 March 2014 19:43, David Rowley dgrowle...@gmail.com wrote: On Thu, Mar 27, 2014 at 7:33 AM, Tom Lane t...@sss.pgh.pa.us wrote: David Rowley dgrowle...@gmail.com writes: I've attached an updated invtrans_strictstrict_base patch which has the feature removed. What is the state of play on this patch? Is the latest version what's in http://www.postgresql.org/message-id/64f96fd9-64d1-40b9-8861-e61820292...@phlo.org plus this sub-patch? Is everybody reasonably happy with it? I don't see it marked ready for committer in the CF app, but time is running out. As far as I know the only concern left was around the extra stats in the explain output, which I removed in the patch I attached in the previous email. Agreed. That was my last concern regarding the base patch, and I agree that removing the new explain output is probably the best course of action, given that we haven't reached consensus as to what the most useful output would be. The invtrans_strictstrict_base.patch in my previous email replaces the invtrans_strictstrict_base_038070.patch in that Florian sent here http://www.postgresql.org/message-id/64f96fd9-64d1-40b9-8861-e61820292...@phlo.org all of the other patches are unchanged so it's save to use Florian's latest ones Perhaps Dean can confirm that there's nothing else outstanding? Florian mentioned upthread that the docs hadn't been updated to reflect the latest changes, so I think they need a little attention. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
On 03/27/2014 11:34 AM, Christoph Berg wrote: Re: Andrew Dunstan 2014-03-27 53344465.6010...@dunslane.net On 03/27/2014 11:27 AM, Tom Lane wrote: Andrew Dunstan and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] four minor proposals for 9.5
Hello 2014-03-20 1:28 GMT+01:00 Josh Berkus j...@agliodbs.com: Pavel, I wrote a few patches, that we use in our production. These patches are small, but I hope, so its can be interesting for upstream: 1. cancel time - we log a execution time cancelled statements Manually cancelled? statement_timeout? Anyway, +1 to add the time to the existing log message, but not in an additional log line. probably it will be possible BTW, what do folks think of the idea of adding a new log column called timing, which would record duration etc.? It would be really nice not to have to parse this out of the text message all the time ... 2. fatal verbose - this patch ensure a verbose log for fatal errors. It simplify a investigation about reasons of error. Configurable, or not? 3. relation limit - possibility to set session limit for maximum size of relations. Any relation cannot be extended over this limit in session, when this value is higher than zero. Motivation - we use lot of queries like CREATE TABLE AS SELECT .. , and some very big results decreased a disk free space too much. It was high risk in our multi user environment. Motivation is similar like temp_files_limit. I'd think the size of the relation you were creating would be difficult to measure. Also, would this apply to REINDEX/VACUUM FULL/ALTER? Or just CREATE TABLE AS/SELECT INTO? 4. track statement lock - we are able to track a locking time for query and print this data in slow query log and auto_explain log. It help to us with lather slow query log analysis. I'm very interested in this. What does it look like? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
Re: [HACKERS] four minor proposals for 9.5
Hello After week, I can to evaluate a community reflection: 2014-03-19 16:34 GMT+01:00 Pavel Stehule pavel.steh...@gmail.com: Hello I wrote a few patches, that we use in our production. These patches are small, but I hope, so its can be interesting for upstream: 1. cancel time - we log a execution time cancelled statements there is a interest 2. fatal verbose - this patch ensure a verbose log for fatal errors. It simplify a investigation about reasons of error. not too much 3. relation limit - possibility to set session limit for maximum size of relations. Any relation cannot be extended over this limit in session, when this value is higher than zero. Motivation - we use lot of queries like CREATE TABLE AS SELECT .. , and some very big results decreased a disk free space too much. It was high risk in our multi user environment. Motivation is similar like temp_files_limit. is not a interest 4. track statement lock - we are able to track a locking time for query and print this data in slow query log and auto_explain log. It help to us with lather slow query log analysis. there is a interest So I'll prepare a some prototypes in next month for 1. log a execution time for cancelled queries, 2. track a query lock time Regards Pavel Do you thinking so these patches can be generally useful? Regards Pavel
Re: [HACKERS] [patch] Adding EXTRA_REGRESS_OPTS to all pg_regress invocations
Re: Bruce Momjian 2013-12-04 20131204151533.gb17...@momjian.us On Mon, May 6, 2013 at 11:51:47PM -0700, Christoph Berg wrote: make check supports EXTRA_REGRESS_OPTS to pass extra options to pg_regress, but all the other places where pg_regress is used do not allow this. The attached patch adds EXTRA_REGRESS_OPTS to Makefile.global.in (for contrib modules) and two more special Makefiles (isolation and pg_upgrade). The use case here is that Debian needs to be able to redirect the unix socket directory used to /tmp, because /var/run/postgresql isn't writable for the buildd user. The matching part for this inside pg_regress is still in discussion here, but the addition of EXTRA_REGRESS_OPTS is an independent step that is also useful for others, so I'd like to propose it for inclusion. Thanks, patch applied. This will appear in PG 9.4. I suppose we could backpatch this but I would need community feedback on that. Thanks for pushing this. In the meantime, a new bit has appeared: The new contrib/test_decoding checks make use of the pg_isolation_regress_check macros (which the isolation test itself doesn't). These macros also need EXTRA_REGRESS_OPTS, on top of 86ef4796f5120c55d1a48cfab52e51df8ed271b5: diff --git a/src/Makefile.global.in b/src/Makefile.global.in new file mode 100644 index cdddf49..8d08d19 *** a/src/Makefile.global.in --- b/src/Makefile.global.in *** pg_regress_installcheck = $(top_builddir *** 468,475 pg_regress_clean_files = results/ regression.diffs regression.out tmp_check/ log/ ! pg_isolation_regress_check = $(top_builddir)/src/test/isolation/pg_isolation_regress --inputdir=$(srcdir) --temp-install=./tmp_check --top-builddir=$(top_builddir) $(pg_regress_locale_flags) ! pg_isolation_regress_installcheck = $(top_builddir)/src/test/isolation/pg_isolation_regress --inputdir=$(srcdir) --top-builddir=$(top_builddir) $(pg_regress_locale_flags) ## # --- 468,475 pg_regress_clean_files = results/ regression.diffs regression.out tmp_check/ log/ ! pg_isolation_regress_check = $(top_builddir)/src/test/isolation/pg_isolation_regress --inputdir=$(srcdir) --temp-install=./tmp_check --top-builddir=$(top_builddir) $(pg_regress_locale_flags) $(EXTRA_REGRESS_OPTS) ! pg_isolation_regress_installcheck = $(top_builddir)/src/test/isolation/pg_isolation_regress --inputdir=$(srcdir) --top-builddir=$(top_builddir) $(pg_regress_locale_flags) $(EXTRA_REGRESS_OPTS) ## # Christoph -- c...@df7cb.de | http://www.df7cb.de/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] using arrays within structure in ECPG
On Mon, Mar 24, 2014 at 11:52:30AM +0530, Ashutosh Bapat wrote: For all the members of struct employee, except arr_col, the size of array is set to 14 and next member offset is set of sizeof (struct employee). But for arr_col they are set to 3 and sizeof(int) resp. So, for the next row onwards, the calculated offset of arr_col member would not coincide with the real arr_col member's address. Am I missing something here? No, this looks like a bug to me. I haven't had time to look into the source codebut the offset definitely is off. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at gmail dot com VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
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 + +++ /home/nmisch/src/pg/postgresql/contrib/test_decoding/results/ddl.out 2014-03-27 17:40:33.079815557 + @@ -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]: data[integer]:- (3 rows) -- Noah Misch EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql \d+ and oid display
On 27-03-2014 10:15, Bruce Momjian wrote: When we made OIDs optional, we added an oid status display to \d+: test= \d+ test Table public.test Column | Type | Modifiers | Storage | Stats target | Description +-+---+-+--+- x | integer | | plain | | -- Has OIDs: no Do we want to continue displaying that OID line, or make it optional for cases where the value doesn't match default_with_oids? That line is still important for those tables that have oids. Once a while I see with oids set (mainly by mistake or misinformation). The 8.0 (last version that have default_with_oids = on) is dead for more than 3 years. Is it time to remove the d_w_o or even announce to remove it in 2 releases? Regarding to remove the Has OIDs line, it seems that it is just noise since almost all tables does not have oids. However, if you remove that line you'll have to fix the regression tests and also can break third part extensions that use \d+ (we can live with that). If we agree to remove d_w_o, don't worry with that line because it will be removed soon. -- Euler Taveira Timbira - http://www.timbira.com.br/ PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql \d+ and oid display
* Euler Taveira (eu...@timbira.com.br) wrote: On 27-03-2014 10:15, Bruce Momjian wrote: When we made OIDs optional, we added an oid status display to \d+: test= \d+ test Table public.test Column | Type | Modifiers | Storage | Stats target | Description +-+---+-+--+- x | integer | | plain | | -- Has OIDs: no Do we want to continue displaying that OID line, or make it optional for cases where the value doesn't match default_with_oids? That line is still important for those tables that have oids. Once a while I see with oids set (mainly by mistake or misinformation). I believe Bruce was suggesting to show it when it is set to *not* the default, which strikes me as perfectly reasonable. If the default is without OIDs, then this will show on tables which *have* OIDs. And vice-versa. The 8.0 (last version that have default_with_oids = on) is dead for more than 3 years. Is it time to remove the d_w_o or even announce to remove it in 2 releases? I don't know that we need to drop the option... Regarding to remove the Has OIDs line, it seems that it is just noise since almost all tables does not have oids. However, if you remove that line you'll have to fix the regression tests and also can break third part extensions that use \d+ (we can live with that). If we agree to remove d_w_o, don't worry with that line because it will be removed soon. I expect there are products which are still using it... Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Doing better at HINTing an appropriate column within errorMissingColumn()
With the addition of LATERAL subqueries, Tom fixed up the mechanism for keeping track of which relations are visible for column references while the FROM clause is being scanned. That allowed errorMissingColumn() to give a more useful error to the one produced by the prior coding of that mechanism, with an errhint sometimes proffering: 'There is a column named foo in table bar, but it cannot be referenced from this part of the query'. I wondered how much further this could be taken. Attached patch modifies contrib/fuzzystrmatch, moving its Levenshtein distance code into core without actually moving the relevant SQL functions too. That change allowed me to modify errorMissingColumn() to make more useful suggestions as to what might have been intended under other circumstances, like when someone fat-fingers a column name. psql tab completion is good, but not so good that this doesn't happen all the time. It's good practice to consistently name columns and tables such that it's possible to intuit the names of columns from the names of tables and so on, but it's still pretty common to forget if a column name from the table orders is order_id, orderid, or ordersid, particularly if you're someone who regularly interacts with many databases. This problem is annoying in a low intensity kind of way. Consider the following sample sessions of mine, made with the dellstore2 sample database: [local]/postgres=# select * from orders o join orderlines ol on o.orderid = ol.orderids limit 1; ERROR: 42703: column ol.orderids does not exist LINE 1: ...* from orders o join orderlines ol on o.orderid = ol.orderid... ^ HINT: Perhaps you meant to reference the column ol.orderid. LOCATION: errorMissingColumn, parse_relation.c:2989 [local]/postgres=# select * from orders o join orderlines ol on o.orderid = ol.orderid limit 1; orderid | orderdate | customerid | netamount | tax | totalamount | orderlineid | orderid | prod_id | quantity | orderdate -+++---+---+-+-+-+-+--+ 1 | 2004-01-27 | 7888 |313.24 | 25.84 | 339.08 | 1 | 1 |9117 |1 | 2004-01-27 (1 row) [local]/postgres=# select ordersid from orders o join orderlines ol on o.orderid = ol.orderid limit 1; ERROR: 42703: column ordersid does not exist LINE 1: select ordersid from orders o join orderlines ol on o.orderi... ^ HINT: Perhaps you meant to reference the column o.orderid. LOCATION: errorMissingColumn, parse_relation.c:2999 [local]/postgres=# select ol.ordersid from orders o join orderlines ol on o.orderid = ol.orderid limit 1; ERROR: 42703: column ol.ordersid does not exist LINE 1: select ol.ordersid from orders o join orderlines ol on o.ord... ^ HINT: Perhaps you meant to reference the column ol.orderid. LOCATION: errorMissingColumn, parse_relation.c:2989 We try to give the most useful possible HINT here, charging extra for a non-matching alias, and going through the range table in order and preferring the first column observed to any subsequent column whose name is of the same distance as an earlier Var. The fuzzy string matching works well enough that it seems possible in practice to successfully have the parser make the right suggestion, even when the user's original guess was fairly far off. I've found it works best to charge half as much for a character deletion, so that's what is charged. I have some outstanding concerns about the proposed patch: * It may be the case that dense logosyllabic or morphographic writing systems, for example Kanji might consistently present, say, Japanese users with a suggestion that just isn't very useful, to the point of being annoying. Perhaps some Japanese hackers can comment on the actual risks here. * Perhaps I should have moved the Levenshtein distance functions into core and be done with it. I thought that given the present restriction that the implementation imposes on source and target string lengths, it would be best to leave the user-facing SQL functions in contrib. That restriction is not relevant to the internal use of Levenshtein distance added here, though. Thoughts? -- Peter Geoghegan levenshtein_column_hint.v1.2014_03_27.patch.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
Andrew Dunstan and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
On 03/27/2014 04:31 PM, Tom Lane wrote: Andrew Dunstan and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql \d+ and oid display
Bruce Momjian wrote When we made OIDs optional, we added an oid status display to \d+: test= \d+ test Table public.test Column | Type | Modifiers | Storage | Stats target | Description +-+---+-+--+- x | integer | | plain | | -- Has OIDs: no Do we want to continue displaying that OID line, or make it optional for cases where the value doesn't match default_with_oids? If we didn't make it behave this way at the time of the change then what has changed that we should make it behave this way now? I like the logic generally but not necessarily the change. The disadvantage of this change is users (both end and tools) of the data now also have to look at what the default is (or was at the time the text was generated) to know what a suppressed OIDs means. Given how much information the typical \d+ generates I would suspect that the added noise this introduces is quickly ignored by frequent users and not all the disruptive to those who use \d+ infrequently. Tools likely would prefer is to be always displayed. My $0.02 David J. -- View this message in context: http://postgresql.1045698.n5.nabble.com/psql-d-and-oid-display-tp5797653p5797707.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql \d+ and oid display
On 03/27/2014 04:43 PM, David Johnston wrote: Bruce Momjian wrote When we made OIDs optional, we added an oid status display to \d+: test= \d+ test Table public.test Column | Type | Modifiers | Storage | Stats target | Description +-+---+-+--+- x | integer | | plain | | -- Has OIDs: no Do we want to continue displaying that OID line, or make it optional for cases where the value doesn't match default_with_oids? If we didn't make it behave this way at the time of the change then what has changed that we should make it behave this way now? I like the logic generally but not necessarily the change. The disadvantage of this change is users (both end and tools) of the data now also have to look at what the default is (or was at the time the text was generated) to know what a suppressed OIDs means. Given how much information the typical \d+ generates I would suspect that the added noise this introduces is quickly ignored by frequent users and not all the disruptive to those who use \d+ infrequently. Tools likely would prefer is to be always displayed. Frankly, to me it's just useless noise. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minimum supported version of Python?
Peter Eisentraut pete...@gmx.net writes: You backpatched this change, but that can't be right, because the feature that requires the cdecimal module was added in 9.4 (7919398bac8bacd75ec5d763ce8b15ffaaa3e071). Ah. I saw that the failing tests were quite old, but did not realize that we'd only recently added this way for them to fail. Will adjust. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Negative Transition Aggregate Functions (WIP)
First, sorry guys for letting this slide - I was overwhelmed by other works, and this kind of slipped my mind :-( On Mar27, 2014, at 09:04 , Dean Rasheed dean.a.rash...@gmail.com wrote: On 26 March 2014 19:43, David Rowley dgrowle...@gmail.com wrote: On Thu, Mar 27, 2014 at 7:33 AM, Tom Lane t...@sss.pgh.pa.us wrote: David Rowley dgrowle...@gmail.com writes: I've attached an updated invtrans_strictstrict_base patch which has the feature removed. What is the state of play on this patch? Is the latest version what's in http://www.postgresql.org/message-id/64f96fd9-64d1-40b9-8861-e61820292...@phlo.org plus this sub-patch? Is everybody reasonably happy with it? I don't see it marked ready for committer in the CF app, but time is running out. As far as I know the only concern left was around the extra stats in the explain output, which I removed in the patch I attached in the previous email. Agreed. That was my last concern regarding the base patch, and I agree that removing the new explain output is probably the best course of action, given that we haven't reached consensus as to what the most useful output would be. After re-reading the thread, I'd prefer to go with Dean's suggestion, i.e. simply reporting the total number of invocations of the forward transition functions, and the total number of invocations of the reverse transition function, over reporting nothing. The labels of the two counts would simply be Forward Transitions and Reverse Transitions. But I don't want this issue to prevent us from getting this patch into 9.4, so if there are objections to this, I'll rip out the EXPLAIN stuff all together. The invtrans_strictstrict_base.patch in my previous email replaces the invtrans_strictstrict_base_038070.patch in that Florian sent here http://www.postgresql.org/message-id/64f96fd9-64d1-40b9-8861-e61820292...@phlo.org all of the other patches are unchanged so it's save to use Florian's latest ones Perhaps Dean can confirm that there's nothing else outstanding? Florian mentioned upthread that the docs hadn't been updated to reflect the latest changes, so I think they need a little attention. I'll see to updating the docs, and will post a final patch within the next few days. Dean, have you by chance looked at the other patches yet? best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql \d+ and oid display
On 2014-03-27 09:15:52 -0400, Bruce Momjian wrote: When we made OIDs optional, we added an oid status display to \d+: test= \d+ test Table public.test Column | Type | Modifiers | Storage | Stats target | Description +-+---+-+--+- x | integer | | plain | | -- Has OIDs: no Do we want to continue displaying that OID line, or make it optional for cases where the value doesn't match default_with_oids? I think we should just leave this alone. Changing this seems useless noise at this point. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
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 + +++ /home/nmisch/src/pg/postgresql/contrib/test_decoding/results/ddl.out 2014-03-27 17:40:33.079815557 + @@ -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]: data[integer]:- (3 rows) That sounds like a good plan. Thanks! Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] History of WAL_LEVEL (archive vs hot_standby)
shamccoy wrote Hello. I've been doing some benchmarks on performance / size differences between actions when wal_level is set to either archive or hot_standby. I'm not seeing a ton of difference. I've read some posts about discussions as to whether this parameter should be simplified and remove or merge these 2 values. I'd like to understand the historic reason between have the extra hot_standby value. Was it to introduce replication and not disturb the already working archive value? If I'm new to Postgres, is there any strategic reason to use archive at this point if replication is something I'll be using in the future? I'm not seeing any downside to hot_standby unless I'm missing something fundamental. Thanks, Shawn There is a semantic difference in that archive is limited to wal shipping to a dead-drop area for future PITR. hot_standby implies that the wal is being sent to another running system that is immediately reading in the information to maintain an exact live copy of the master. As I think both can be used for PITR I don't believe there is much downside, technically or with resources, to using hot_standby instead of archive; but I do not imagine it having any practical benefit either. David J. -- View this message in context: http://postgresql.1045698.n5.nabble.com/History-of-WAL-LEVEL-archive-vs-hot-standby-tp5797717p5797720.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Cube extension kNN support
Hi everyone, On Sun, Sep 22, 2013 at 4:38 PM, Stas Kelvich stas.kelv...@gmail.com wrote: Here is the patch that introduces kNN search for cubes with euclidean, taxicab and chebyshev distances. What is the status of this patch? -- Kind regards, Sergey Konoplev PostgreSQL Consultant and DBA http://www.linkedin.com/in/grayhemp +1 (415) 867-9984, +7 (901) 903-0499, +7 (988) 888-1979 gray...@gmail.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] History of WAL_LEVEL (archive vs hot_standby)
On 03/27/2014 03:06 PM, David Johnston wrote: As I think both can be used for PITR I don't believe there is much downside, technically or with resources, to using hot_standby instead of archive; but I do not imagine it having any practical benefit either. Actually, hot_standby does have to write some extra records to the WAL which archive does not. I don't know that anyone has checked the actual volume difference between the two, though, which would probably also vary by workload. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless Replica Identity: NOTHING noise from psql \d
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=crakedt=2014-03-27%2023%3A40%3A15stg=test-decoding-check. Larger questions can wait, but this makes a start on the immediate issue. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] History of WAL_LEVEL (archive vs hot_standby)
On Thu, Mar 27, 2014 at 03:06:02PM -0700, David Johnston wrote: shamccoy wrote Hello. I've been doing some benchmarks on performance / size differences between actions when wal_level is set to either archive or hot_standby. I'm not seeing a ton of difference. I've read some posts about discussions as to whether this parameter should be simplified and remove or merge these 2 values. I'd like to understand the historic reason between have the extra hot_standby value. Was it to introduce replication and not disturb the already working archive value? I think so. If I'm new to Postgres, is there any strategic reason to use archive at this point if replication is something I'll be using in the future? I'm not seeing any downside to hot_standby unless I'm missing something fundamental. Probably not. You might manage to construct a benchmark in which the extra master-side work is measurable, but it wouldn't be easy. There is a semantic difference in that archive is limited to wal shipping to a dead-drop area for future PITR. hot_standby implies that the wal is being sent to another running system that is immediately reading in the information to maintain an exact live copy of the master. As I think both can be used for PITR I don't believe there is much downside, technically or with resources, to using hot_standby instead of archive; but I do not imagine it having any practical benefit either. wal_level=archive vs. hot_standby is orthogonal to the mechanism for transmitting WAL and time of applying WAL. Rather, it dictates whether a PostgreSQL cluster replaying that WAL can accept read-only connections during recovery. You can send wal_level=archive log data over streaming replication, even synchronous streaming replication. However, any standby will be unable to accept connections until failover ends recovery. On the flip side, if you use wal_level=hot_standby, a backup undergoing PITR can accept read-only connections while applying years-old WAL from an archive. That is useful for verifying, before you end recovery, that you have replayed far enough. -- Noah Misch EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] PATCH: decreasing memory needlessly consumed by array_agg
Hi,
this is a patch for issue reported in October 2013 in pgsql-bugs:
http://www.postgresql.org/message-id/3839201.nfa2rvc...@techfox.foxi
Frank van Vugt reported that a simple query with array_agg() and large
number of groups (1e7) fails because of OOM even on machine with 32GB of
RAM.
So for example doing this:
CREATE TABLE test (a INT, b INT);
INSERT INTO test SELECT i, i FROM generate_series(1,1000) s(i);
SELECT a, array_agg(b) FROM test GROUP BY a;
allocates huge amounts of RAM and easily forces the machine into
swapping and eventually gets killed by OOM (on my workstation with 8GB
of RAM that happens almost immediately).
Upon investigation, it seems caused by a combination of issues:
(1) per-group memory contexts - each group state uses a dedicated
memory context, which is defined like this (in accumArrayResult):
arr_context = AllocSetContextCreate(rcontext,
accumArrayResult,
ALLOCSET_DEFAULT_MINSIZE,
ALLOCSET_DEFAULT_INITSIZE,
ALLOCSET_DEFAULT_MAXSIZE);
which actually means this
arr_context = AllocSetContextCreate(rcontext,
accumArrayResult,
0,
(8*1024),
(8*1024*1024));
so each group will allocate at least 8kB of memory (of the first
palloc call). With 1e7 groups, that's ~80GB of RAM, even if each
group contains just 1 item.
(2) minimum block size in aset.c - The first idea I got was to decrease
the block size in the allocator. So I decreased it to 256B but I
was still getting OOM. Then I found that aset.c contains this:
if (initBlockSize 1024)
initBlockSize = 1024;
so effectively the lowest allowed block size is 1kB. Which means
~10GB of memory for the state data (i.e. not considering overhead
of the hash table etc., which is not negligible).
Considering we're talking about 1e7 32-bit integers, i.e. 40MB
of raw data, that's still pretty excessive (250x more).
While I question whether the 1kB minimum block size makes sense, I
really think per-group memory contexts don't make much sense here. What
is the point of per-group memory contexts?
The memory will be allocated when the first row of the group is
received, and won't be allocated until the whole result set is
processed. At least that's how it works for Hash Aggregate.
However that's exactly how it would work with a single memory context,
which has the significant benefit that all the groups share the same
memory (so the minimum block size is not an issue).
That is exactly what the patch aims to do - it removes the per-group
memory contexts and reuses the main memory context of the aggregate
itself.
The patch also does one more thing - it changes how the arrays (in the
aggregate state) grow. Originally it worked like this
/* initial size */
astate-alen = 64;
/* when full, grow exponentially */
if (astate-nelems = astate-alen)
astate-alen *= 2;
so the array length would grow like this 64 - 128 - 256 - 512 ...
(note we're talking about elements, not bytes, so with with 32-bit
integers it's actually 256B - 512B - 1024B - ...).
While I do understand the point of this (minimizing palloc overhead), I
find this pretty dangerous, especially in case of array_agg(). I've
modified the growth like this:
/* initial size */
astate-alen = 4;
/* when full, grow exponentially */
if (astate-nelems = astate-alen)
astate-alen += 4;
I admit that might be a bit too aggressive, and maybe there's a better
way to do this - with better balance between safety and speed. I was
thinking about something like this:
/* initial size */
astate-alen = 4;
/* when full, grow exponentially */
if (astate-nelems = astate-alen)
if (astate-alen 128)
astate-alen *= 2;
else
astate-alen += 128;
i.e. initial size with exponential growth, but capped at 128B.
regards
Tomas
diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index 91df184..ef86cac 100644
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -4577,16 +4577,10 @@ accumArrayResult(ArrayBuildState *astate,
{
/* First time through --- initialize */
- /* Make a temporary context to hold all the junk */
- arr_context = AllocSetContextCreate(rcontext,
- accumArrayResult,
- ALLOCSET_DEFAULT_MINSIZE,
- ALLOCSET_DEFAULT_INITSIZE,
- ALLOCSET_DEFAULT_MAXSIZE);
- oldcontext = MemoryContextSwitchTo(arr_context);
+ oldcontext = MemoryContextSwitchTo(rcontext);
astate = (ArrayBuildState *) palloc(sizeof(ArrayBuildState));
- astate-mcontext = arr_context;
- astate-alen = 64; /* arbitrary starting array size */
+ astate-mcontext = rcontext;
+ astate-alen = 4; /* arbitrary starting array size */
astate-dvalues = (Datum *) palloc(astate-alen * sizeof(Datum));
Re: [HACKERS] History of WAL_LEVEL (archive vs hot_standby)
Noah Misch-2 wrote On Thu, Mar 27, 2014 at 03:06:02PM -0700, David Johnston wrote: shamccoy wrote Hello. I've been doing some benchmarks on performance / size differences between actions when wal_level is set to either archive or hot_standby. I'm not seeing a ton of difference. I've read some posts about discussions as to whether this parameter should be simplified and remove or merge these 2 values. I'd like to understand the historic reason between have the extra hot_standby value. Was it to introduce replication and not disturb the already working archive value? I think so. If I'm new to Postgres, is there any strategic reason to use archive at this point if replication is something I'll be using in the future? I'm not seeing any downside to hot_standby unless I'm missing something fundamental. Probably not. You might manage to construct a benchmark in which the extra master-side work is measurable, but it wouldn't be easy. There is a semantic difference in that archive is limited to wal shipping to a dead-drop area for future PITR. hot_standby implies that the wal is being sent to another running system that is immediately reading in the information to maintain an exact live copy of the master. As I think both can be used for PITR I don't believe there is much downside, technically or with resources, to using hot_standby instead of archive; but I do not imagine it having any practical benefit either. wal_level=archive vs. hot_standby is orthogonal to the mechanism for transmitting WAL and time of applying WAL. Rather, it dictates whether a PostgreSQL cluster replaying that WAL can accept read-only connections during recovery. You can send wal_level=archive log data over streaming replication, even synchronous streaming replication. However, any standby will be unable to accept connections until failover ends recovery. On the flip side, if you use wal_level=hot_standby, a backup undergoing PITR can accept read-only connections while applying years-old WAL from an archive. That is useful for verifying, before you end recovery, that you have replayed far enough. Went looking for this in the docs... http://www.postgresql.org/docs/9.3/interactive/runtime-config-wal.html#GUC-WAL-LEVEL So I guess, no-restore/offline/online would be good names (and maybe wal_restore_mode instead of wal_level) if we started from scratch. Note that no-restore does not preclude same-system recovery. Just something to help me remember... David J. -- View this message in context: http://postgresql.1045698.n5.nabble.com/History-of-WAL-LEVEL-archive-vs-hot-standby-tp5797717p5797733.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] History of WAL_LEVEL (archive vs hot_standby)
David Johnston wrote Noah Misch-2 wrote On Thu, Mar 27, 2014 at 03:06:02PM -0700, David Johnston wrote: shamccoy wrote Hello. I've been doing some benchmarks on performance / size differences between actions when wal_level is set to either archive or hot_standby. I'm not seeing a ton of difference. I've read some posts about discussions as to whether this parameter should be simplified and remove or merge these 2 values. I'd like to understand the historic reason between have the extra hot_standby value. Was it to introduce replication and not disturb the already working archive value? I think so. If I'm new to Postgres, is there any strategic reason to use archive at this point if replication is something I'll be using in the future? I'm not seeing any downside to hot_standby unless I'm missing something fundamental. Probably not. You might manage to construct a benchmark in which the extra master-side work is measurable, but it wouldn't be easy. There is a semantic difference in that archive is limited to wal shipping to a dead-drop area for future PITR. hot_standby implies that the wal is being sent to another running system that is immediately reading in the information to maintain an exact live copy of the master. As I think both can be used for PITR I don't believe there is much downside, technically or with resources, to using hot_standby instead of archive; but I do not imagine it having any practical benefit either. wal_level=archive vs. hot_standby is orthogonal to the mechanism for transmitting WAL and time of applying WAL. Rather, it dictates whether a PostgreSQL cluster replaying that WAL can accept read-only connections during recovery. You can send wal_level=archive log data over streaming replication, even synchronous streaming replication. However, any standby will be unable to accept connections until failover ends recovery. On the flip side, if you use wal_level=hot_standby, a backup undergoing PITR can accept read-only connections while applying years-old WAL from an archive. That is useful for verifying, before you end recovery, that you have replayed far enough. Went looking for this in the docs... http://www.postgresql.org/docs/9.3/interactive/runtime-config-wal.html#GUC-WAL-LEVEL So I guess, no-restore/offline/online would be good names (and maybe wal_restore_mode instead of wal_level) if we started from scratch. Note that no-restore does not preclude same-system recovery. Just something to help me remember... David J. Slightly tangential but are the locking operations associated with the recent bugfix generated in both (all?) modes or only hot_standby? I thought it strange that transient locking operations were output with WAL though I get it if they are there to support read-only queries. David J. -- View this message in context: http://postgresql.1045698.n5.nabble.com/History-of-WAL-LEVEL-archive-vs-hot-standby-tp5797717p5797735.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
Hi, ForeignPath *pathnode = makeNode(ForeignPath); + Assert(rel-rtekind == RTE_RELATION); pathnode-path.pathtype = T_ForeignScan; .. Maybe I'm missing the point, but I don't think it'd be better to put the assertion in create_foreignscan_path(). And I think it'd be the caller' responsiblity to ensure that equality, as any other pathnode creation routine for a baserel in pathnode.c assumes that equality. Hmm. The assertion (not shown above but you put in parameterize_path:) seems to say that 'base relation for foreign paths must be a RTE_RELATION' isn't right? But I don't see anything putting such a restriction in reparameterize_path itself. Could you tell me where such a restriction comes from? Or who needs such a restriction? I think any assertions shouldn't be anywhere other than where just before needed. Thoughts? regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Archive recovery won't be completed on some situation.
Hello, After all, pmState changes to PM_NO_CHILDREN via PM_WAIT_DEAD_END by SIGCHLDs from non-significant processes, then CancelBackup(). Judging from what was being said on the thread, it seems that running CancelBackup() after an immediate shutdown is better than not doing it, correct? Agreed. I like that behavior:) It removes backup_label at immediate shutdown and (altough I didn't see by myself but as far as I saw PostmasterStateMachine) it would skip shutdown checkponit. Focusing on the point described above, the small patch below rewinds the behavior back to 9.3 and before but I don't know the appropriateness in regard to the intention of the patch. I see. Obviously your patch would, in effect, revert 82233ce7ea completely, which is not something we want. I think if we want to go back to the previous behavior of not stopping the backup, some other method should be used. As I mentioned above, I don't want to rewind 9.4's behavior back to that of previous ones. What I hope to be realized for now is '-b'(provisional optname) of pg_resetxlog for at least versions which would fall into this problem. What do you think about this maybe 'New Feature' but has meaning practically only for older versions? Of course I agree with that 'you should erase the backup_label just after master has crashed' is the most clean and sane way to *avoid* the situation but the penalty seems a bit too large for the mistake. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Archive recovery won't be completed on some situation.
Hello, At Wed, 19 Mar 2014 19:34:10 +0900, Fujii Masao wrote Agreed. Attached patches do that and I could recover the database state with following steps, Adding new option looks like new feature rather than bug fix. I'm afraid that the backpatch of such a change to 9.3 or before is not acceptable. Me too. But on the other hand it simplly is a relief for the consequence of the behavior of server (altough it was ill operation:), and especially it is needed for at least 9.1 which seems cannot be saved without it. Plus it has utterly no impact on servers' behavior of any corresponding versions. So I hope it is accepted. Even in 9.1, we can think that problematic situation as database corruption and restart the server from the backup which was successfully taken before. No? Mmm. I don't think it is relevant to this problem. The problem specific here is 'The database was running until just now, but shutdown the master (by pacemaker), then restart, won't run anymore'. Deleting backup_label after immediate shutdown is the radical measure but existing system would be saved by the option. But, honestly saying, I (also?) don't have sympathy for the situation so much and if all or most of you think the option can cause another problem, I won't insist about that any more. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] History of WAL_LEVEL (archive vs hot_standby)
Hi, Went looking for this in the docs... http://www.postgresql.org/docs/9.3/interactive/runtime-config-wal.html#GUC-WAL-LEVEL So I guess, no-restore/offline/online would be good names (and maybe wal_restore_mode instead of wal_level) if we started from scratch. Note that no-restore does not preclude same-system recovery. Just something to help me remember... David J. Slightly tangential but are the locking operations associated with the recent bugfix generated in both (all?) modes or only hot_standby? I thought it strange that transient locking operations were output with WAL though I get it if they are there to support read-only queries. Putting aside the naming:), I have caught by the discussion about the differences of wal records to be emitted among the wal levels. I grep'ed 'wal_level' for whole backend but all it showed was for checking of some options in postgresql.conf against other options in postgresql.conf and that in control file. None of them seems to care it for the purpose of controlling how/what wal records to emit or record construction, except for WAL_LEVEL_LOGICAL. As far as I could see, I doubt that there is any difference in emitted wal records amoung wal levels, (except for logical changeset). I came to want to try to run streaming replication with wal_level = minimal but no time for now:( regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
