Re: [HACKERS] UNNEST with multiple args, and TABLE with multiple funcs
On 08/20/2013 02:03 AM, Josh Berkus wrote: On 08/19/2013 09:23 AM, Boszormenyi Zoltan wrote: Indeed, it's a big nail in the coffin for SRFs-in-targetlist. Having WITH ORDINALITY and this feature, I would vote for removing SRF-in-targetlist and call the release PostgreSQL 10.0. That's not realistic. We'd have to deprecate that syntax and repeatedly and loudly warn people about it for at least 3 years after the release of 9.3. You're talking about asking people to refactor hundreds or thousands of lines of code which makes current use of things like regex_match() in the target list. Agreed. Even three years is optimistic; after that long it could probably be made into an ERROR by default with a backward-compat GUC, but certainly not removed. I'm still running into people running 8.2 and having issues upgrading due to the 8.3 removal of implicit casts from text, and even the removal of add_missing_from . If we want people to upgrade this century it's worth minimising the amount of unnecessary breakage. SRF-in-SELECT might be ugly, but simply ripping it out certainly counts as unnecessary breakage. -- Craig Ringer 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] UNNEST with multiple args, and TABLE with multiple funcs
On Mon, Aug 19, 2013 at 07:45:23PM +0200, Pavel Stehule wrote: Hello Harder maybe but it may still be cleaner in the long run. Overall, it's my intention here to remove as many as feasible of the old reasons why one might use an SRF in the select list. Indeed, it's a big nail in the coffin for SRFs-in-targetlist. Having WITH ORDINALITY and this feature, I would vote for removing SRF-in-targetlist and call the release PostgreSQL 10.0. Although I would to remove SRF from targetlist, I don't think so this hurry strategy is good idea. We should to provide new functionality and old functionality one year as minimum, and we should to announce so this feature is deprecated We could do this in 9.3, but all it would be is an announcement, i.e. no code change of any nature. - and maybe use a GUC for disabling, warning and deprecating. With utmost respect, I think the general idea of setting SQL grammar via GUC is a really bad one. When we've done so in the past, it's done more harm than good, and we should not repeat it. More, I would to see 9.4 release:). Same here! :) x.4 are happy PostgreSQL releases :) Each one has been at least baseline happy for me since 7.1. Some have made me overjoyed, though. Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- 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] UNNEST with multiple args, and TABLE with multiple funcs
2013/8/20 David Fetter da...@fetter.org On Mon, Aug 19, 2013 at 07:45:23PM +0200, Pavel Stehule wrote: Hello Harder maybe but it may still be cleaner in the long run. Overall, it's my intention here to remove as many as feasible of the old reasons why one might use an SRF in the select list. Indeed, it's a big nail in the coffin for SRFs-in-targetlist. Having WITH ORDINALITY and this feature, I would vote for removing SRF-in-targetlist and call the release PostgreSQL 10.0. Although I would to remove SRF from targetlist, I don't think so this hurry strategy is good idea. We should to provide new functionality and old functionality one year as minimum, and we should to announce so this feature is deprecated We could do this in 9.3, but all it would be is an announcement, i.e. no code change of any nature. - and maybe use a GUC for disabling, warning and deprecating. With utmost respect, I think the general idea of setting SQL grammar via GUC is a really bad one. When we've done so in the past, it's done more harm than good, and we should not repeat it. so as minumum is controlling warning via GUC, we should to help with identification of problematic queries. Regards Pavel More, I would to see 9.4 release:). Same here! :) x.4 are happy PostgreSQL releases :) Each one has been at least baseline happy for me since 7.1. Some have made me overjoyed, though. Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: [HACKERS] UNNEST with multiple args, and TABLE with multiple funcs
2013-08-20 08:13 keltezéssel, Pavel Stehule írta: 2013/8/20 David Fetter da...@fetter.org mailto:da...@fetter.org On Mon, Aug 19, 2013 at 07:45:23PM +0200, Pavel Stehule wrote: Hello Harder maybe but it may still be cleaner in the long run. Overall, it's my intention here to remove as many as feasible of the old reasons why one might use an SRF in the select list. Indeed, it's a big nail in the coffin for SRFs-in-targetlist. Having WITH ORDINALITY and this feature, I would vote for removing SRF-in-targetlist and call the release PostgreSQL 10.0. Although I would to remove SRF from targetlist, I don't think so this hurry strategy is good idea. We should to provide new functionality and old functionality one year as minimum, and we should to announce so this feature is deprecated We could do this in 9.3, but all it would be is an announcement, i.e. no code change of any nature. - and maybe use a GUC for disabling, warning and deprecating. To really ensure backward compatibility, this sentence should read as add a GUC for disabling *the* warning and deprecating. :- As I said, I am such an agent provocateur. Let this side track die and concentrate on the merits of the patch itself. :-) With utmost respect, I think the general idea of setting SQL grammar via GUC is a really bad one. When we've done so in the past, it's done more harm than good, and we should not repeat it. so as minumum is controlling warning via GUC, we should to help with identification of problematic queries. Regards Pavel More, I would to see 9.4 release:). Same here! :) x.4 are happy PostgreSQL releases :) Each one has been at least baseline happy for me since 7.1. Some have made me overjoyed, though. Cheers, David. -- David Fetter da...@fetter.org mailto:da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 tel:%2B1%20415%20235%203778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com mailto:david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics http://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/
Re: [HACKERS] ereport documentation patch
On 19.08.2013 23:40, Christophe Pettus wrote: Is it reasonable to note in the documentation that ereport does not return if the error severity is greater than or equal to ERROR? Yeah, it probably would be good to mention that. Got a patch? - 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] Backup throttling
On 19.08.2013 21:15, Boszormenyi Zoltan wrote: 2013-08-19 19:20 keltezéssel, Andres Freund írta: Based on a quick look it seems like you're throttling on the receiving side. Is that a good idea? Especially over longer latency links, TCP buffering will reduce the effect on the sender side considerably. Throttling on the sender side requires extending the syntax of BASE_BACKUP and maybe START_REPLICATION so both can be throttled but throttling is still initiated by the receiver side. Throttling in the client seems much better to me. TCP is designed to handle a slow client. Maybe throttling the walsender is not a good idea, it can lead to DoS via disk space shortage. If a client can initiate a backup and/or streaming replication, he can already do much more damage than a DoS via out of disk space. And a nothing stops even a non-privileged user from causing an out of disk space situation anyway. IOW that's a non-issue. - 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] Backup throttling
2013-08-20 08:37 keltezéssel, Heikki Linnakangas írta: On 19.08.2013 21:15, Boszormenyi Zoltan wrote: 2013-08-19 19:20 keltezéssel, Andres Freund írta: Based on a quick look it seems like you're throttling on the receiving side. Is that a good idea? Especially over longer latency links, TCP buffering will reduce the effect on the sender side considerably. Throttling on the sender side requires extending the syntax of BASE_BACKUP and maybe START_REPLICATION so both can be throttled but throttling is still initiated by the receiver side. Throttling in the client seems much better to me. TCP is designed to handle a slow client. Maybe throttling the walsender is not a good idea, it can lead to DoS via disk space shortage. If a client can initiate a backup and/or streaming replication, he can already do much more damage than a DoS via out of disk space. And a nothing stops even a non-privileged user from causing an out of disk space situation anyway. IOW that's a non-issue. I got to the same conclusion this morning, but because of wal_keep_segments. Best regards, Zoltán Böszörményi - Heikki -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ -- 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] Personal note: taking some vacation time in Sep/Oct
On 20/08/13 15:26, Tom Lane wrote: I will be taking a long (and long-overdue) vacation from Sep 10 to Oct 20. I expect to have email access, but won't be doing much more than minimally keeping up with my inbox. This means I'll be pretty much AWOL for the September commitfest :-(. That's unfortunate, but the dates for this trip were frozen long before the 9.4 development calendar was. regards, tom lane but, But, BUT, you're not human - you can't possibly take leave, the sky will fall all manners of divers calamities will come to pass!!! MORE SERIOUSLY: Enjoy your more than well earnt leave! Cheers, Gavin
Re: [HACKERS] StrategyGetBuffer optimization, take 2
On 2013-08-19 15:17:44 -0700, Jeff Janes wrote: On Wed, Aug 7, 2013 at 7:40 AM, Merlin Moncure mmonc...@gmail.com wrote: I agree; at least then it's not unambiguously better. if you (in effect) swap all contention on allocation from a lwlock to a spinlock it's not clear if you're improving things; it would have to be proven and I'm trying to keep things simple. Attached is a scaled down version of the patch that keeps the freelist lock but still removes the spinlock during the clock sweep. This still hits the major objectives of reducing the chance of scheduling out while holding the BufFreelistLock and mitigating the worst case impact of doing so if it does happen. An even more scaled down version would keep the current logic exactly as is except for replacing buffer lock in the clock sweep with a trylock (which is IMNSHO a no-brainer). Since usage_count is unsigned, are you sure that changing the tests from buf-usage_count == 0 to buf-usage_count = 0 accomplishes what you need it to? If usage_count gets decremented when it already zero, it will wrap around to 65,535, at least on some compilers some of the time, won't it? Overflow of *unsigned* variables is actually defined and will always wrap around. It's signed variables which don't have such a clear behaviour. 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
[HACKERS] possible enhancing of UPDATE syntax?
Hello we support syntax for INSERT and DELETE statements, that allows a simple triggers for these statements. CREATE FUNCTION trg_insert() RETURNS trigger AS $$ BEGIN INSERT INTO target_tbl VALUES(NEW.*) RETURN NULL; END; $$ LANGUAGE plpgsql; * is not allowed in DELETE, but we can use a virtual column CREATE FUNCTION trg_delete() RETURNS trigger AS $$ BEGIN DELETE FROM target_tbl WHERE target_tbl = OLD; RETURN NULL; END; $$ LANGUAGE plpgsql; Same functionality is missing for UPDATE, although we support (targetlist) = (ROW) syntax So should be nice and consistent with previous behave UPDATE target_tbl SET (target_tbl.*) = (NEW) WHERE target_tbl = OLD; What do you think about it? Regards Pavel
Re: [HACKERS] undefined symbol: PQescapeLiteral
You're linking against a pre-9.0 copy of libpq.so. Thanks for help :) I got my mistake . Regards, Amul Sul -- View this message in context: http://postgresql.1045698.n5.nabble.com/undefined-symbol-PQescapeLiteral-tp5767578p5767994.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
[HACKERS] Parsing bool type value
Hi all, Taking a look at PostgreSQL HEAD today, I noticed that currently PostgreSQL allows of value as bool type value. So user can execute the following SQL. =# SET enbale_seqscan TO of; And I read the source code related to parsing bool value. It compare TWO characters off and the setting value in parse_bool_with_len() function. Should we deny the of value as bool type value? Regards, --- Sawada Masahiko parse_bool_with_len.patch Description: Binary 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] Extension Templates S03E11
Hi,
2013-08-04 15:20 keltezéssel, Dimitri Fontaine írta:
Thom Brown t...@linux.com writes:
Could you please resubmit this without using SnapshotNow as it's no longer
supported?
Sure, sorry that I missed that, please find v12 attached.
Here's a review for this patch.
* Is the patch in a patch format which has context? (eg: context diff format)
Yes.
* Does it apply cleanly to the current git master?
No, it has one reject in src/bin/pg_dump/pg_backup_archiver.c.
It was obvious to fix, version 12a is attached.
* Does it include reasonable tests, necessary doc patches, etc?
It has extended the SQL reference nicely but the reference to
xref linkend=extend-extensions was not obvious enough
regarding the list of control parameters.
The SQL syntax has them in allcaps, while the referenced section
has them in lower case. It's easy to miss them while just browsing
for e.g. RELOCATABLE. I had to go back twice to find the proper
part of the text.
I would like to see the control parameters documented in allcaps
in CREATE EXTENSION TEMPLATE. Then ALTER EXTENSION
TEMPLATE should reference the CREATE instead of the longer
text in xref linkend=extend-extensions. This xref can still
be there for reference in all three of CREATE/ALTER/DROP
EXTENSION TEMPLATE.
* Does the patch actually implement what it's supposed to do?
Yes.
* Do we want that?
Yes.
* Do we already have it?
No.
* Does it follow SQL spec, or the community-agreed behavior?
There's no such provision in the spec.
As far as I can tell, it follows the community-agreed behavior.
* Does it include pg_dump support (if applicable)?
Yes.
But the version check is already wrong in src/bin/pg_dump/pg_dump.c
since this patch missed 9.3.
+ /*
+* Before 9.3, there are no extension templates.
+*/
+ if (fout-remoteVersion 90300)
+ {
+ *numExtensionTemplates = 0;
+ return NULL;
+ }
+
* Are there dangers?
I don't think so.
* Have all the bases been covered?
It seems so.
* Does the feature work as advertised?
Yes.
* Are there corner cases the author has failed to consider?
I don't know.
* Are there any assertion failures or crashes?
No.
* Does the patch slow down simple tests?
No.
* If it claims to improve performance, does it?
n/a
* Does it slow down other things?
No.
* Does it follow the project coding guidelines?
Yes.
Nitpicking. This chunk has an extra unnecessary space:
diff --git a/src/backend/catalog/Makefile b/src/backend/catalog/Makefile
index c4d3f3c..689dc37 100644
--- a/src/backend/catalog/Makefile
+++ b/src/backend/catalog/Makefile
@@ -38,6 +38,7 @@ POSTGRES_BKI_SRCS = $(addprefix
$(top_srcdir)/src/include/catalog/,\
pg_authid.h pg_auth_members.h pg_shdepend.h pg_shdescription.h \
pg_ts_config.h pg_ts_config_map.h pg_ts_dict.h \
pg_ts_parser.h pg_ts_template.h pg_extension.h \
+pg_extension_control.h pg_extension_template.h pg_extension_uptmpl.h \
pg_foreign_data_wrapper.h pg_foreign_server.h pg_user_mapping.h \
pg_foreign_table.h \
pg_default_acl.h pg_seclabel.h pg_shseclabel.h pg_collation.h
pg_range.h \
* Are there portability issues?
No.
* Will it work on Windows/BSD etc?
It should.
* Are the comments sufficient and accurate?
Yes.
* Does it do what it says, correctly?
According to the added regression tests, yes.
* Does it produce compiler warnings?
No.
* Can you make it crash?
No.
* Is everything done in a way that fits together coherently with other
features/modules?
I think so.
* Are there interdependencies that can cause problems?
I don't know.
Best regards,
Zoltán Böszörményi
--
--
Zoltán Böszörményi
Cybertec Schönig Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
templates.v12a.patch.gz
Description: Unix tar archive
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] PL/pgSQL PERFORM with CTE
Hackers, This seems reasonable: david=# DO $$ david$# BEGIN david$# WITH now AS (SELECT now()) david$# SELECT * from now; david$# END; david$# $$; ERROR: query has no destination for result data HINT: If you want to discard the results of a SELECT, use PERFORM instead. CONTEXT: PL/pgSQL function inline_code_block line 3 at SQL statement This not so much: david=# DO $$ david$# BEGIN david$# WITH now AS (SELECT now()) david$# PERFORM * from now; david$# END; david$# $$; ERROR: syntax error at or near PERFORM LINE 4: PERFORM * from now; ^ Parser bug in PL/pgSQL, perhaps? Best, David -- 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] PL/pgSQL PERFORM with CTE
Hello 2013/8/20 David E. Wheeler da...@justatheory.com Hackers, This seems reasonable: david=# DO $$ david$# BEGIN david$# WITH now AS (SELECT now()) david$# SELECT * from now; david$# END; david$# $$; ERROR: query has no destination for result data HINT: If you want to discard the results of a SELECT, use PERFORM instead. CONTEXT: PL/pgSQL function inline_code_block line 3 at SQL statement This not so much: david=# DO $$ david$# BEGIN david$# WITH now AS (SELECT now()) david$# PERFORM * from now; david$# END; david$# $$; ERROR: syntax error at or near PERFORM LINE 4: PERFORM * from now; ^ Parser bug in PL/pgSQL, perhaps? no you cannot use a PL/pgSQL statement inside SQL statement. Regards Pavel Best, David -- 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] PL/pgSQL PERFORM with CTE
Hi Pavel, On Aug 20, 2013, at 2:11 PM, Pavel Stehule pavel.steh...@gmail.com wrote: david=# DO $$ david$# BEGIN david$# WITH now AS (SELECT now()) david$# PERFORM * from now; david$# END; david$# $$; ERROR: syntax error at or near PERFORM LINE 4: PERFORM * from now; ^ Parser bug in PL/pgSQL, perhaps? no you cannot use a PL/pgSQL statement inside SQL statement. Well, there ought to be *some* way to tell PL/pgSQL to discard the result. Right now I am adding a variable to select into but never otherwise use. Inelegant, IMHO. Perhaps I’m missing some other way to do it? If so, it would help if the hint suggesting the use of PERFORM pointed to such alternatives. Best, David -- 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] PL/pgSQL PERFORM with CTE
2013/8/20 David E. Wheeler da...@justatheory.com Hi Pavel, On Aug 20, 2013, at 2:11 PM, Pavel Stehule pavel.steh...@gmail.com wrote: david=# DO $$ david$# BEGIN david$# WITH now AS (SELECT now()) david$# PERFORM * from now; david$# END; david$# $$; ERROR: syntax error at or near PERFORM LINE 4: PERFORM * from now; ^ Parser bug in PL/pgSQL, perhaps? no you cannot use a PL/pgSQL statement inside SQL statement. Well, there ought to be *some* way to tell PL/pgSQL to discard the result. Right now I am adding a variable to select into but never otherwise use. Inelegant, IMHO. Perhaps I’m missing some other way to do it? If so, it would help if the hint suggesting the use of PERFORM pointed to such alternatives. postgres=# DO $$ BEGIN PERFORM * FROM (WITH now AS (SELECT now()) SELECT * from now) x; END; $$; DO postgres=# Regards Pavel Best, David
Re: [HACKERS] PL/pgSQL PERFORM with CTE
On 2013-08-20 14:15:55 +0200, David E. Wheeler wrote: Hi Pavel, On Aug 20, 2013, at 2:11 PM, Pavel Stehule pavel.steh...@gmail.com wrote: david=# DO $$ david$# BEGIN david$# WITH now AS (SELECT now()) david$# PERFORM * from now; david$# END; david$# $$; ERROR: syntax error at or near PERFORM LINE 4: PERFORM * from now; ^ Parser bug in PL/pgSQL, perhaps? no you cannot use a PL/pgSQL statement inside SQL statement. Well, there ought to be *some* way to tell PL/pgSQL to discard the result. Right now I am adding a variable to select into but never otherwise use. Inelegant, IMHO. Perhaps I’m missing some other way to do it? If so, it would help if the hint suggesting the use of PERFORM pointed to such alternatives. Not that that's elegant but IIRC PERFORM (WITH ...) ought to work. I don't think the intermingled plpgsql/sql grammars allow a nice way right now. 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] PL/pgSQL PERFORM with CTE
On 8/20/13 2:21 PM, Pavel Stehule wrote: 2013/8/20 David E. Wheeler da...@justatheory.com Well, there ought to be *some* way to tell PL/pgSQL to discard the result. Right now I am adding a variable to select into but never otherwise use. Inelegant, IMHO. Perhaps I’m missing some other way to do it? If so, it would help if the hint suggesting the use of PERFORM pointed to such alternatives. postgres=# DO $$ BEGIN PERFORM * FROM (WITH now AS (SELECT now()) SELECT * from now) x; END; $$; DO .. which doesn't work if you want to use table-modifying CTEs. Regards, Marko Tiikkaja -- 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] PL/pgSQL PERFORM with CTE
2013/8/20 Andres Freund and...@2ndquadrant.com On 2013-08-20 14:15:55 +0200, David E. Wheeler wrote: Hi Pavel, On Aug 20, 2013, at 2:11 PM, Pavel Stehule pavel.steh...@gmail.com wrote: david=# DO $$ david$# BEGIN david$# WITH now AS (SELECT now()) david$# PERFORM * from now; david$# END; david$# $$; ERROR: syntax error at or near PERFORM LINE 4: PERFORM * from now; ^ Parser bug in PL/pgSQL, perhaps? no you cannot use a PL/pgSQL statement inside SQL statement. Well, there ought to be *some* way to tell PL/pgSQL to discard the result. Right now I am adding a variable to select into but never otherwise use. Inelegant, IMHO. Perhaps I’m missing some other way to do it? If so, it would help if the hint suggesting the use of PERFORM pointed to such alternatives. Not that that's elegant but IIRC PERFORM (WITH ...) ought to work. I don't think the intermingled plpgsql/sql grammars allow a nice way right now. +1 Pavel Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] PL/pgSQL PERFORM with CTE
On Aug 20, 2013, at 2:24 PM, Marko Tiikkaja ma...@joh.to wrote: postgres=# DO $$ BEGIN PERFORM * FROM (WITH now AS (SELECT now()) SELECT * from now) x; END; $$; DO .. which doesn't work if you want to use table-modifying CTEs. Which, in fact, is exactly my use case (though not what I posted upthread). Best, David -- 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] PL/pgSQL PERFORM with CTE
2013/8/20 David E. Wheeler da...@justatheory.com On Aug 20, 2013, at 2:24 PM, Marko Tiikkaja ma...@joh.to wrote: postgres=# DO $$ BEGIN PERFORM * FROM (WITH now AS (SELECT now()) SELECT * from now) x; END; $$; DO .. which doesn't work if you want to use table-modifying CTEs. Which, in fact, is exactly my use case (though not what I posted upthread). but it works postgres=# do $$begin with x as (select 10) insert into omega select * from x; end;$$; DO Regards Pavel Best, David
Re: [HACKERS] PL/pgSQL PERFORM with CTE
On Aug 20, 2013, at 2:31 PM, Pavel Stehule pavel.steh...@gmail.com wrote: but it works postgres=# do $$begin with x as (select 10) insert into omega select * from x; end;$$; DO But this does not: david=# DO $$ david$# BEGIN david$# PERFORM * FROM ( david$# WITH inserted AS ( david$# INSERT INTO foo values (1) RETURNING id david$# ) SELECT inserted.id david$# ) x; david$# END; david$# $$; ERROR: WITH clause containing a data-modifying statement must be at the top level LINE 2: WITH inserted AS ( ^ QUERY: SELECT * FROM ( WITH inserted AS ( INSERT INTO foo values (1) RETURNING id ) SELECT inserted.id ) x CONTEXT: PL/pgSQL function inline_code_block line 3 at PERFORM Best, David -- 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] PL/pgSQL PERFORM with CTE
2013/8/20 David E. Wheeler da...@justatheory.com On Aug 20, 2013, at 2:31 PM, Pavel Stehule pavel.steh...@gmail.com wrote: but it works postgres=# do $$begin with x as (select 10) insert into omega select * from x; end;$$; DO But this does not: david=# DO $$ david$# BEGIN david$# PERFORM * FROM ( david$# WITH inserted AS ( david$# INSERT INTO foo values (1) RETURNING id david$# ) SELECT inserted.id david$# ) x; david$# END; david$# $$; ERROR: WITH clause containing a data-modifying statement must be at the top level LINE 2: WITH inserted AS ( ^ QUERY: SELECT * FROM ( WITH inserted AS ( INSERT INTO foo values (1) RETURNING id ) SELECT inserted.id ) x CONTEXT: PL/pgSQL function inline_code_block line 3 at PERFORM yes, in this context you should not use a PERFORM PL/pgSQL protect you before useless queries - so you can use a CTE without returned result directly or CTE with result via PERFORM statement (and in this case it must be unmodifing CTE). Sorry, I don't see any problem - why you return some from CTE and then you throw this result? Best, David
Re: [HACKERS] PL/pgSQL PERFORM with CTE
On Tue, Aug 20, 2013 at 7:25 AM, Andres Freund and...@2ndquadrant.com wrote: On 2013-08-20 14:15:55 +0200, David E. Wheeler wrote: Hi Pavel, On Aug 20, 2013, at 2:11 PM, Pavel Stehule pavel.steh...@gmail.com wrote: david=# DO $$ david$# BEGIN david$# WITH now AS (SELECT now()) david$# PERFORM * from now; david$# END; david$# $$; ERROR: syntax error at or near PERFORM LINE 4: PERFORM * from now; ^ Parser bug in PL/pgSQL, perhaps? no you cannot use a PL/pgSQL statement inside SQL statement. Well, there ought to be *some* way to tell PL/pgSQL to discard the result. Right now I am adding a variable to select into but never otherwise use. Inelegant, IMHO. Perhaps I’m missing some other way to do it? If so, it would help if the hint suggesting the use of PERFORM pointed to such alternatives. Not that that's elegant but IIRC PERFORM (WITH ...) ought to work. I don't think the intermingled plpgsql/sql grammars allow a nice way right now. I think the way forward is to remove the restriction such that data returning queries must be PERFORM'd. merlin -- 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] PL/pgSQL PERFORM with CTE
2013/8/20 Merlin Moncure mmonc...@gmail.com On Tue, Aug 20, 2013 at 7:25 AM, Andres Freund and...@2ndquadrant.com wrote: On 2013-08-20 14:15:55 +0200, David E. Wheeler wrote: Hi Pavel, On Aug 20, 2013, at 2:11 PM, Pavel Stehule pavel.steh...@gmail.com wrote: david=# DO $$ david$# BEGIN david$# WITH now AS (SELECT now()) david$# PERFORM * from now; david$# END; david$# $$; ERROR: syntax error at or near PERFORM LINE 4: PERFORM * from now; ^ Parser bug in PL/pgSQL, perhaps? no you cannot use a PL/pgSQL statement inside SQL statement. Well, there ought to be *some* way to tell PL/pgSQL to discard the result. Right now I am adding a variable to select into but never otherwise use. Inelegant, IMHO. Perhaps I’m missing some other way to do it? If so, it would help if the hint suggesting the use of PERFORM pointed to such alternatives. Not that that's elegant but IIRC PERFORM (WITH ...) ought to work. I don't think the intermingled plpgsql/sql grammars allow a nice way right now. I think the way forward is to remove the restriction such that data returning queries must be PERFORM'd I disagree, current rule has sense. Pavel merlin
Re: [HACKERS] PL/pgSQL PERFORM with CTE
On Aug 20, 2013, at 2:41 PM, Pavel Stehule pavel.steh...@gmail.com wrote: yes, in this context you should not use a PERFORM PL/pgSQL protect you before useless queries - so you can use a CTE without returned result directly or CTE with result via PERFORM statement (and in this case it must be unmodifing CTE). Sorry, I don't see any problem - why you return some from CTE and then you throw this result? I am passing the values returned from a CTE to a call to pg_notify(). I do not care to collect the output of pg_notify(), which returns VOID. Best, David -- 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] PL/pgSQL PERFORM with CTE
2013-08-20 14:35 keltezéssel, David E. Wheeler írta: On Aug 20, 2013, at 2:31 PM, Pavel Stehule pavel.steh...@gmail.com wrote: but it works postgres=# do $$begin with x as (select 10) insert into omega select * from x; end;$$; DO But this does not: david=# DO $$ david$# BEGIN david$# PERFORM * FROM ( david$# WITH inserted AS ( david$# INSERT INTO foo values (1) RETURNING id david$# ) SELECT inserted.id david$# ) x; david$# END; david$# $$; ERROR: WITH clause containing a data-modifying statement must be at the top level LINE 2: WITH inserted AS ( ^ QUERY: SELECT * FROM ( WITH inserted AS ( INSERT INTO foo values (1) RETURNING id ) SELECT inserted.id ) x CONTEXT: PL/pgSQL function inline_code_block line 3 at PERFORM This is the same error as if you put the WITH into a subquery, which is what PERFORM does. Proof: SELECT * FROM ( WITH inserted AS ( INSERT INTO foo values (1) RETURNING id ) SELECT inserted.id ) x; Best regards, Zoltán Böszörményi -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ -- 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] PL/pgSQL PERFORM with CTE
On Aug 20, 2013, at 2:44 PM, Pavel Stehule pavel.steh...@gmail.com wrote: I think the way forward is to remove the restriction such that data returning queries must be PERFORM'd I disagree, current rule has sense. Perhaps a DECLARE FUNCTION attribute that turns off the functionality, then? Best, David -- 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] PL/pgSQL PERFORM with CTE
On Tue, Aug 20, 2013 at 7:44 AM, Pavel Stehule pavel.steh...@gmail.com wrote: 2013/8/20 Merlin Moncure mmonc...@gmail.com On Tue, Aug 20, 2013 at 7:25 AM, Andres Freund and...@2ndquadrant.com wrote: On 2013-08-20 14:15:55 +0200, David E. Wheeler wrote: Hi Pavel, On Aug 20, 2013, at 2:11 PM, Pavel Stehule pavel.steh...@gmail.com wrote: david=# DO $$ david$# BEGIN david$# WITH now AS (SELECT now()) david$# PERFORM * from now; david$# END; david$# $$; ERROR: syntax error at or near PERFORM LINE 4: PERFORM * from now; ^ Parser bug in PL/pgSQL, perhaps? no you cannot use a PL/pgSQL statement inside SQL statement. Well, there ought to be *some* way to tell PL/pgSQL to discard the result. Right now I am adding a variable to select into but never otherwise use. Inelegant, IMHO. Perhaps I’m missing some other way to do it? If so, it would help if the hint suggesting the use of PERFORM pointed to such alternatives. Not that that's elegant but IIRC PERFORM (WITH ...) ought to work. I don't think the intermingled plpgsql/sql grammars allow a nice way right now. I think the way forward is to remove the restriction such that data returning queries must be PERFORM'd I disagree, current rule has sense. Curious what your thinking is there. merlin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
* Amit Kapila (amit.kapil...@gmail.com) wrote: Okay, let us decide how things will be if we disable by default: 1. initdb won't create any empty auto file, rather the file will be created first time user uses ALTER SYSTEM. I'm not particularly set on this.. Why not create the file initially? 2. Alter system should issue warning, if it detects that feature is disabled (for this we need to error detection code during parsing of postgresql.conf as it was previously) I agree that it should complain through a warning or perhaps an error (to be honest, I like error more, but there may be good reasons against that). 3. postgresql.conf will contain include directive in below form: #include = 'postgresql.auto.conf' Whenever user wants to use Alter System, he needs to enable it after first time using ALTER SYSTEM. That I don't like.. You should be able to enable it and have things work without having to run ALTER SYSTEM first.. One question here, if somebody just enables it without using ALTER SYSTEM, should it throw any error on not finding auto file or just ignore it? I'd prefer that it error if it can't find an included file. What about include directives? Sorry, I didn't get which parameters you are referring by include directives? Literally, the above 'include' in postgresql.conf. Would that only be dealt with as a parse-time thing, or should it be more like a GUC which could then be set through ALTER SYSTEM? Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] PL/pgSQL PERFORM with CTE
2013/8/20 David E. Wheeler da...@justatheory.com On Aug 20, 2013, at 2:41 PM, Pavel Stehule pavel.steh...@gmail.com wrote: yes, in this context you should not use a PERFORM PL/pgSQL protect you before useless queries - so you can use a CTE without returned result directly or CTE with result via PERFORM statement (and in this case it must be unmodifing CTE). Sorry, I don't see any problem - why you return some from CTE and then you throw this result? I am passing the values returned from a CTE to a call to pg_notify(). I do not care to collect the output of pg_notify(), which returns VOID. it is little bit different issue - PL/pgSQL doesn't check if returned type is VOID - it can be allowed, I am thinking. So check of empty result can be enhanced. Regards Pavel Best, David
Re: [HACKERS] PL/pgSQL PERFORM with CTE
On 8/20/13 2:53 PM, Pavel Stehule wrote: 2013/8/20 David E. Wheeler da...@justatheory.com I am passing the values returned from a CTE to a call to pg_notify(). I do not care to collect the output of pg_notify(), which returns VOID. it is little bit different issue - PL/pgSQL doesn't check if returned type is VOID - it can be allowed, I am thinking. So check of empty result can be enhanced. That still doesn't help at all in the case where the function returns something, but you simply don't care about the result. That said, I don't think this issue is big enough to start radically changing how SELECT without INTO works -- you can always get around this limitation by SELECTing into a variable, as David mentioned in his original message. It's annoying, but it works. Regards, Marko Tiikkaja -- 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] PL/pgSQL PERFORM with CTE
On Aug 20, 2013, at 2:53 PM, Pavel Stehule pavel.steh...@gmail.com wrote: I am passing the values returned from a CTE to a call to pg_notify(). I do not care to collect the output of pg_notify(), which returns VOID. it is little bit different issue - PL/pgSQL doesn't check if returned type is VOID - it can be allowed, I am thinking. So check of empty result can be enhanced. I am confused. I do not need to check the result (except via FOUND). But I am sure I can think of other situations where I am calling something where I do not care about the result, even if it returns one. Best, David -- 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] PL/pgSQL PERFORM with CTE
2013/8/20 David E. Wheeler da...@justatheory.com On Aug 20, 2013, at 2:53 PM, Pavel Stehule pavel.steh...@gmail.com wrote: I am passing the values returned from a CTE to a call to pg_notify(). I do not care to collect the output of pg_notify(), which returns VOID. it is little bit different issue - PL/pgSQL doesn't check if returned type is VOID - it can be allowed, I am thinking. So check of empty result can be enhanced. I am confused. I do not need to check the result (except via FOUND). But I am sure I can think of other situations where I am calling something where I do not care about the result, even if it returns one. When you would to ignore result, then you should to use a PERFORM - actually, it is limited now and should be fixed. Have no problem with it. I don't would to enable a free unbound statement that returns result. Regards Pavel Best, David
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Amit Kapila escribió: 3. postgresql.conf will contain include directive in below form: #include = 'postgresql.auto.conf' Whenever user wants to use Alter System, he needs to enable it after first time using ALTER SYSTEM. This seems wrong to me. If the auto file is read by an include line in postgresql.conf, what is its priority w.r.t. files placed in an hypothetical conf.d directory? This could be handled by a simple ordering with last one wins. I don't particularly like that though.. My gut feeling is that I'd like something to complain if there's more than one value set for the same GUC.. Hopefully snippets put in conf.d/ by puppet/chef will override the settings in postgresql.conf (i.e. conf.d/ should be processed after postgresql.conf, not before); and hopefully ALTER SYSTEM will in turn override conf.d. I see no way to have ALTER SYSTEM handled by an include line, yet still have it override conf.d. With includedir and include directives, and postgresql.conf setting a defined ordering, with last-wins, you could simply have the 'includedir' for conf.d come before the 'include' for auto.conf. If we want to make ALTER SYSTEM disable-able from postgresql.conf, I think it should be an explicit option, something like enable_alter_system = on or something like that. I really don't like this approach- I'd much rather we have a general include mechanism than special alter-system GUCs. Thanks, Stephen signature.asc Description: Digital signature
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
On Tue, Aug 20, 2013 at 6:21 PM, Stephen Frost sfr...@snowman.net wrote: * Amit Kapila (amit.kapil...@gmail.com) wrote: Okay, let us decide how things will be if we disable by default: 1. initdb won't create any empty auto file, rather the file will be created first time user uses ALTER SYSTEM. I'm not particularly set on this.. Why not create the file initially? If by default this feature needs to be disabled, then it is not must to have at initdb time. OTOH if we want to enable it by default, then we need to get this created at initdb time else it will give error during system startup (an error can occur during startup when it will parse postgresql.conf and didn't find the file mentioned in include directive). Also you mentioned below line upthread which I understood as you don't like idea of creating empty file at initdb time: If it's enabled by default, then we need to ship an 'auto' file which is empty by default... I don't particularly like that 2. Alter system should issue warning, if it detects that feature is disabled (for this we need to error detection code during parsing of postgresql.conf as it was previously) I agree that it should complain through a warning or perhaps an error (to be honest, I like error more, but there may be good reasons against that). 3. postgresql.conf will contain include directive in below form: #include = 'postgresql.auto.conf' Whenever user wants to use Alter System, he needs to enable it after first time using ALTER SYSTEM. That I don't like.. You should be able to enable it and have things work without having to run ALTER SYSTEM first.. This was actually linked to point 1 mentioned above, if we create empty file at initdb time, then there should not be any problem. One other option to disable as suggested by Alvaro is have another config parameter to enable/disable Alter System, if we can do that way, the solution will be much neater. One question here, if somebody just enables it without using ALTER SYSTEM, should it throw any error on not finding auto file or just ignore it? I'd prefer that it error if it can't find an included file. What about include directives? Sorry, I didn't get which parameters you are referring by include directives? Literally, the above 'include' in postgresql.conf. Would that only be dealt with as a parse-time thing, or should it be more like a GUC which could then be set through ALTER SYSTEM? I think if we choose to use include directive as a way to enable/disable this feature, it will not be good to allow change of this parameter with Alter System. With Regards, Amit Kapila. 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] PL/pgSQL PERFORM with CTE
On Aug 20, 2013, at 3:05 PM, Pavel Stehule pavel.steh...@gmail.com wrote: When you would to ignore result, then you should to use a PERFORM - actually, it is limited now and should be fixed. Have no problem with it. Glad to have you on board. :-) I don't would to enable a free unbound statement that returns result. I have no pony in that race. I think it is useful, though I prefer to unit test things enough that I would be fine without it. But even without it, there may be times when I want to discard a result in a function that *does* return a value -- likely a different value. So there needs to be a way to distinguish statements that should return a value and those that do not. Best, David -- 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] PL/pgSQL PERFORM with CTE
2013/8/20 David E. Wheeler da...@justatheory.com On Aug 20, 2013, at 3:05 PM, Pavel Stehule pavel.steh...@gmail.com wrote: When you would to ignore result, then you should to use a PERFORM - actually, it is limited now and should be fixed. Have no problem with it. Glad to have you on board. :-) I don't would to enable a free unbound statement that returns result. I have no pony in that race. I think it is useful, though I prefer to unit test things enough that I would be fine without it. But even without it, there may be times when I want to discard a result in a function that *does* return a value -- likely a different value. So there needs to be a way to distinguish statements that should return a value and those that do not. can you show some examples, please Pavel Best, David
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
* Amit Kapila (amit.kapil...@gmail.com) wrote: On Tue, Aug 20, 2013 at 6:21 PM, Stephen Frost sfr...@snowman.net wrote: I'm not particularly set on this.. Why not create the file initially? If by default this feature needs to be disabled, then it is not must to have at initdb time. I don't see how the two are related. You could never use two-phase commit (could even disable it), yet we still create things in the data directory to support it. Having a file in the data directory which isn't used by default isn't that big a deal, imv. Also you mentioned below line upthread which I understood as you don't like idea of creating empty file at initdb time: If it's enabled by default, then we need to ship an 'auto' file which is empty by default... I don't particularly like that What I didn't like was having an empty file be accepted as 'valid', but you need to have some kind of bootstrap mechanism. Telling users run this command and then ignore the warning is certainly bad. A better option might be to have a *non-empty* auto.conf be generated, where all it has in it is some kind of identifier, perhaps even just enable_alter_system = on which we could then key off of to see if ALTER SYSTEM has been set up to be allowed or not. I think if we choose to use include directive as a way to enable/disable this feature, it will not be good to allow change of this parameter with Alter System. I agree, but then we need to add it to the list of things ALTER SYSTEM can't modify (if the include is implemented as a GUC, that is; I've not looked). Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] PL/pgSQL PERFORM with CTE
On Aug 20, 2013, at 3:18 PM, Pavel Stehule pavel.steh...@gmail.com wrote: can you show some examples, please This is not dissimilar to what I am actually doing: CREATE TABLE foo (id SERIAL PRIMARY KEY, name TEXT); CREATE OR REPLACE FUNCTION shipit ( VARIADIC things TEXT[] ) RETURNS BOOL LANGUAGE plpgsql AS $$ BEGIN WITH inserted AS ( INSERT INTO foo (name) SELECT * FROM unnest(things) RETURNING id ) PERFORM pg_notify( 'inserted ids', ARRAY(SELECT * FROM inserted)::text ); RETURN FOUND; END; $$; Only I am using a dummy row variable instead of PERFORM, of course. Best, David -- 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] GSOC13 proposal - extend RETURNING syntax
Hi,
2013-08-19 21:52 keltezéssel, Boszormenyi Zoltan írta:
2013-08-19 21:21 keltezéssel, Karol Trzcionka írta:
W dniu 19.08.2013 19:56, Boszormenyi Zoltan pisze:
* Does it apply cleanly to the current git master?
No. There's a reject in src/backend/optimizer/plan/initsplan.c
Thank you, merged in attached version.
* Does it include reasonable tests?
Yes but the test fails after trying to fix the rejected chunk of the
patch.
It fails because the HINT was changed, fixed.
That version merges some nested ifs left over from earlier work.
I tried to compile your v5 patch and I got:
initsplan.c: In function ‘add_vars_to_targetlist’:
initsplan.c:216:26: warning: ‘rel’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
rel-reltargetlist = lappend(rel-reltargetlist,
^
You shouldn't change the assignment at declaration:
- RelOptInfo *rel = find_base_rel(root, var-varno);
+ RelOptInfo *rel;
...
+ if (root-parse-commandType == CMD_UPDATE)
+ {
... (code using rel)
+ }
+ rel = find_base_rel(root, varno);
Let me say it again: the new code in initsplan.c::add_vars_to_targetlist() is
fishy.
The compiler says that rel used on line 216 may be uninitialized.
Keeping it that way passes make check, perhaps rel was initialized
in a previous iteration of foreach(temp, vars), possibly in the
else if (IsA(node, PlaceHolderVar))
branch, which means that PlaceHolderInfo *phinfo may be interpreted
as RelOptInfo *, stomping on memory.
Moving the assignment back to the declaration makes make check
fail with the attached regression.diffs file.
Initializing it as RelOptInfo *rel = NULL; makes the regression check
die with a segfault, obviously.
Change the code to avoid the warning and still produce the wanted effect.
Best regards,
Zoltán Böszörményi
--
--
Zoltán Böszörményi
Cybertec Schönig Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
***
/home/zozo/crosscolumn/review/returning-before-after/postgresql/src/test/regress/expected/returning_before_after.out
2013-08-20 15:01:03.365314482 +0200
---
/home/zozo/crosscolumn/review/returning-before-after/postgresql/src/test/regress/results/returning_before_after.out
2013-08-20 15:30:21.091641454 +0200
***
*** 6,47
);
INSERT INTO foo VALUES (1, 'x'),(2,'y');
UPDATE foo SET bar1=bar1+1 RETURNING before.*, bar1, bar2;
! bar1 | bar2 | bar1 | bar2
! --+--+--+--
! 1 | x|2 | x
! 2 | y|3 | y
! (2 rows)
!
UPDATE foo SET bar1=bar1-1 RETURNING after.bar1, before.bar1*2;
! bar1 | ?column?
! --+--
! 1 |4
! 2 |6
! (2 rows)
!
UPDATE foo SET bar1=bar1+1, bar2=bar2 || 'z' RETURNING before.*, after.*;
! bar1 | bar2 | bar1 | bar2
! --+--+--+--
! 1 | x|2 | xz
! 2 | y|3 | yz
! (2 rows)
!
-- check single after
UPDATE foo SET bar1=bar1+1, bar2=bar2 || 'a' RETURNING after.*;
! bar1 | bar2
! --+--
! 3 | xza
! 4 | yza
! (2 rows)
!
-- check single before
UPDATE foo SET bar1=bar1+1, bar2=bar2 || 'b' RETURNING before.*;
! bar1 | bar2
! --+--
! 3 | xza
! 4 | yza
! (2 rows)
!
-- it should fail
UPDATE foo SET bar1=bar1+before.bar1 RETURNING before.*;
ERROR: missing FROM-clause entry for table before
--- 6,22
);
INSERT INTO foo VALUES (1, 'x'),(2,'y');
UPDATE foo SET bar1=bar1+1 RETURNING before.*, bar1, bar2;
! ERROR: no relation entry for relid 2
UPDATE foo SET bar1=bar1-1 RETURNING after.bar1, before.bar1*2;
! ERROR: no relation entry for relid 3
UPDATE foo SET bar1=bar1+1, bar2=bar2 || 'z' RETURNING before.*, after.*;
! ERROR: no relation entry for relid 2
-- check single after
UPDATE foo SET bar1=bar1+1, bar2=bar2 || 'a' RETURNING after.*;
! ERROR: no relation entry for relid 3
-- check single before
UPDATE foo SET bar1=bar1+1, bar2=bar2 || 'b' RETURNING before.*;
! ERROR: no relation entry for relid 2
-- it should fail
UPDATE foo SET bar1=bar1+before.bar1 RETURNING before.*;
ERROR: missing FROM-clause entry for table before
***
*** 53,90
^
-- test before/after aliases
UPDATE foo AS before SET bar1=bar1+1 RETURNING before.*,after.*;
! bar1 | bar2 | bar1 | bar2
! --+--+--+--
! 5 | xzab |5 | xzab
! 6 | yzab |6 | yzab
! (2 rows)
!
UPDATE foo AS after SET bar1=bar1-1 RETURNING before.*,after.*;
! bar1 | bar2 | bar1 | bar2
! --+--+--+--
! 5 | xzab |4 | xzab
! 6 | yzab |5 | yzab
! (2 rows)
!
-- test inheritance
CREATE TABLE foo2 (bar INTEGER) INHERITS(foo);
INSERT INTO foo2 VALUES (1,'b',5);
UPDATE foo2 SET bar1=bar1*2, bar=bar1+5, bar2=bar1::text || bar::text
RETURNING before.*, after.*, *;
! bar1 | bar2 | bar | bar1 | bar2 | bar | bar1 | bar2 | bar
!
Re: [HACKERS] PL/pgSQL PERFORM with CTE
2013/8/20 David E. Wheeler da...@justatheory.com On Aug 20, 2013, at 3:18 PM, Pavel Stehule pavel.steh...@gmail.com wrote: can you show some examples, please This is not dissimilar to what I am actually doing: CREATE TABLE foo (id SERIAL PRIMARY KEY, name TEXT); CREATE OR REPLACE FUNCTION shipit ( VARIADIC things TEXT[] ) RETURNS BOOL LANGUAGE plpgsql AS $$ BEGIN WITH inserted AS ( INSERT INTO foo (name) SELECT * FROM unnest(things) RETURNING id ) PERFORM pg_notify( 'inserted ids', ARRAY(SELECT * FROM inserted)::text ); RETURN FOUND; END; $$; Only I am using a dummy row variable instead of PERFORM, of course. pg_notify returns void, so there are no necessary casting to void so enhanced check - so all returned columns are void should be enough Regards Pavel Best, David
Re: [HACKERS] Fix Windows socket error checking for MinGW
On 08/19/2013 11:36 PM, Michael Cronenworth wrote: On 08/19/2013 07:35 PM, Noah Misch wrote: That was option #1. (You weren't planning to change just the one symbol causing the failure at hand, were you?) Reasonable choice. The point in the code comment quoted above looks bad, but the lack of reports of that nature against official 9.2 binaries corroborates it having not been a problem yet. The only non-socket use I see for the constants in question is the EINTR test in XLogWrite(), which probably can't happen on Windows. Redefining compiler constants is bad bandaid. A similar bandaid was in place to begin with caused this problem. If you believe PostgreSQL will never use code that needs the true errno value then go ahead with Option 1. No the reverse is the case. The real problem is that the bandaid was not applied sufficiently widely. What we propose is exactly what is already in use for the Microsoft compilers, and has thus been well and truly tested over some years. Changing these definitions doesn't change the value of errno in the slightest - it only changes the values that we test for. The caveat in the MSVC-specific code that Noah pointed to still applies, but it appears that we don't in fact use these constants anywhere other than in relation to sockets. I'm about to update my buildfarm member jacana so it has the latest mingw-w64 compiler and this should exhibit this error. Then I'll apply a patch along the lines of option 1. If nothing breaks, I'll backpatch that to 9.1 where we enabled use of this compiler. 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] PL/pgSQL PERFORM with CTE
On Aug 20, 2013, at 3:38 PM, Pavel Stehule pavel.steh...@gmail.com wrote: pg_notify returns void, so there are no necessary casting to void so enhanced check - so all returned columns are void should be enough What if I call another function I wrote myself that returns an INT, but I do not care about the INT? Maybe that function does the insert and returns the number of inserted rows. I can think of all kinds of reasons this might be the case; whether they are good or bad approaches is immaterial: sometimes you work with what you have. I am find with PERFORM to determine when a query's results should be discarded. I just think it needs to cover a few more cases. Best, David -- 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] PL/pgSQL PERFORM with CTE
2013/8/20 David E. Wheeler da...@justatheory.com On Aug 20, 2013, at 3:38 PM, Pavel Stehule pavel.steh...@gmail.com wrote: pg_notify returns void, so there are no necessary casting to void so enhanced check - so all returned columns are void should be enough What if I call another function I wrote myself that returns an INT, but I do not care about the INT? Maybe that function does the insert and returns the number of inserted rows. I can think of all kinds of reasons this might be the case; whether they are good or bad approaches is immaterial: sometimes you work with what you have. I am find with PERFORM to determine when a query's results should be discarded. I just think it needs to cover a few more cases. yes I understand. I'll look, how PERFORM can be fixed Regards Pavel Best, David
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
On Tue, Aug 20, 2013 at 6:55 PM, Stephen Frost sfr...@snowman.net wrote: * Amit Kapila (amit.kapil...@gmail.com) wrote: On Tue, Aug 20, 2013 at 6:21 PM, Stephen Frost sfr...@snowman.net wrote: I'm not particularly set on this.. Why not create the file initially? If by default this feature needs to be disabled, then it is not must to have at initdb time. I don't see how the two are related. You could never use two-phase commit (could even disable it), yet we still create things in the data directory to support it. Having a file in the data directory which isn't used by default isn't that big a deal, imv. Yes, it can be done, what I wanted to say it is not a must, rather a good to have thing. Also you mentioned below line upthread which I understood as you don't like idea of creating empty file at initdb time: If it's enabled by default, then we need to ship an 'auto' file which is empty by default... I don't particularly like that What I didn't like was having an empty file be accepted as 'valid', but you need to have some kind of bootstrap mechanism. Telling users run this command and then ignore the warning is certainly bad. A better option might be to have a *non-empty* auto.conf be generated, where all it has in it is some kind of identifier, perhaps even just enable_alter_system = on which we could then key off of to see if ALTER SYSTEM has been set up to be allowed or not. Wouldn't it be complicated to handle this way rather then by having include directive. If include directive is enabled then the autofile will be read else no need to read it. If we want to have separate identifier to judge ALTER SYSTEM is enable or not, then it is better to go with a new GUC. I think if we choose to use include directive as a way to enable/disable this feature, it will not be good to allow change of this parameter with Alter System. I agree, but then we need to add it to the list of things ALTER SYSTEM can't modify (if the include is implemented as a GUC, that is; I've not looked). I have checked and it seems to be just parse time thing. With Regards, Amit Kapila. 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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
* Amit Kapila (amit.kapil...@gmail.com) wrote: Wouldn't it be complicated to handle this way rather then by having include directive. You misunderstood. I was suggesting a setup along these lines: postgresql.conf: # include = 'auto.conf' # Disabled by default auto.conf: # MANAGED BY ALTER SYSTEM -- DO NOT MODIFY BY HAND # auto.conf not processed unless included by postgresql.conf # If included by postgresql.conf, then this will turn on the # ALTER SYSTEM command. enable_alter_system = on Which would give us the ability to independently turn on/off ALTER SYSTEM from including auto.conf. Thanks, Stephen signature.asc Description: Digital signature
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
On Tue, Aug 20, 2013 at 7:51 PM, Stephen Frost sfr...@snowman.net wrote: * Amit Kapila (amit.kapil...@gmail.com) wrote: Wouldn't it be complicated to handle this way rather then by having include directive. You misunderstood. I was suggesting a setup along these lines: postgresql.conf: # include = 'auto.conf' # Disabled by default auto.conf: # MANAGED BY ALTER SYSTEM -- DO NOT MODIFY BY HAND # auto.conf not processed unless included by postgresql.conf # If included by postgresql.conf, then this will turn on the # ALTER SYSTEM command. enable_alter_system = on Which would give us the ability to independently turn on/off ALTER SYSTEM from including auto.conf. So let me try to explain what I understood from above: 1. enable_alter_system a new GUC whose default value =off. 2. Alter System will check this variable and return error (not allowed), if this parameter is off. 3. Now if user enables include directive in postgresql.conf, it will enable Alter System as value of enable_alter_system is on. 4. User can run Alter System command to disable Alter System enable_alter_system = off. Now even though include directive is enabled, but new Alter System commands will not work, however existing parameter's take into effect on restart/sighup. With Regards, Amit Kapila. 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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
* Amit Kapila (amit.kapil...@gmail.com) wrote: So let me try to explain what I understood from above: 1. enable_alter_system a new GUC whose default value =off. 2. Alter System will check this variable and return error (not allowed), if this parameter is off. 3. Now if user enables include directive in postgresql.conf, it will enable Alter System as value of enable_alter_system is on. 4. User can run Alter System command to disable Alter System enable_alter_system = off. Now even though include directive is enabled, but new Alter System commands will not work, however existing parameter's take into effect on restart/sighup. Yes. Not sure that it'd be terribly likely for a user to do that, but if they do it, so be it. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
Thank you for the review and tests. New version introduce a lot of
improvements:
- Fix regression test for view (wrong table_name)
- Add regression test for inheritance
- Delete hack in initsplan.c (now we ignore all RTE_BEFORE) - the
uninitialized issue
- Revert changing varno in add_vars_to_targetlist
- Add all before variables to targetlist
- Avoid adding variables to slot for AFTER.
- Treat varnoold like a flag - prevent from adjustment if RTE_BEFORE
- All before/after are now set on OUTER_VAR
- Rename fix_varno_varattno to bind_returning_variables
- Add comment about bind_returning_variables
- Remove unneeded code in fix_join_expr_mutator (it was changing varno
of RTE_BEFORE - now there is not any var with varno assigned to it)
Regards,
Karol Trzcionka
diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml
index 90b9208..eba35f0 100644
--- a/doc/src/sgml/ref/update.sgml
+++ b/doc/src/sgml/ref/update.sgml
@@ -194,12 +194,27 @@ UPDATE [ ONLY ] replaceable class=PARAMETERtable_name/replaceable [ * ] [
termreplaceable class=PARAMETERoutput_expression/replaceable/term
listitem
para
- An expression to be computed and returned by the commandUPDATE/
- command after each row is updated. The expression can use any
- column names of the table named by replaceable class=PARAMETERtable_name/replaceable
- or table(s) listed in literalFROM/.
- Write literal*/ to return all columns.
+ An expression to be computed and returned by the
+ commandUPDATE/ command either before or after (prefixed with
+ literalBEFORE./literal and literalAFTER./literal,
+ respectively) each row is updated. The expression can use any
+ column names of the table named by replaceable
+ class=PARAMETERtable_name/replaceable or table(s) listed in
+ literalFROM/. Write literalAFTER.*/literal to return all
+ columns after the update. Write literalBEFORE.*/literal for all
+ columns before the update. Write literal*/literal to return all
+ columns after update and all triggers fired (these values are in table
+ after command). You may combine BEFORE, AFTER and raw columns in the
+ expression.
/para
+ warningpara
+ Mixing table names or aliases named before or after with the
+ above will result in confusion and suffering. If you happen to
+ have a table called literalbefore/literal or
+ literalafter/literal, alias it to something else when using
+ RETURNING.
+ /para/warning
+
/listitem
/varlistentry
@@ -287,15 +302,16 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
/para
para
- Perform the same operation and return the updated entries:
+ Perform the same operation and return information on the changed entries:
programlisting
UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
WHERE city = 'San Francisco' AND date = '2003-07-03'
- RETURNING temp_lo, temp_hi, prcp;
+ RETURNING temp_lo AS new_low, temp_hi AS new_high, BEFORE.temp_hi/BEFORE.temp_low AS old_ratio, AFTER.temp_hi/AFTER.temp_low AS new_ratio prcp;
/programlisting
/para
+
para
Use the alternative column-list syntax to do the same update:
programlisting
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index d86e9ad..fafd311 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2335,7 +2335,7 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
TupleTableSlot *
ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
ResultRelInfo *relinfo,
- ItemPointer tupleid, TupleTableSlot *slot)
+ ItemPointer tupleid, TupleTableSlot *slot, TupleTableSlot **planSlot)
{
TriggerDesc *trigdesc = relinfo-ri_TrigDesc;
HeapTuple slottuple = ExecMaterializeSlot(slot);
@@ -2381,6 +2381,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
if (newSlot != NULL)
{
slot = ExecFilterJunk(relinfo-ri_junkFilter, newSlot);
+ *planSlot = newSlot;
slottuple = ExecMaterializeSlot(slot);
newtuple = slottuple;
}
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 15f5dcc..06ebaf3 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -603,7 +603,7 @@ ExecUpdate(ItemPointer tupleid,
resultRelInfo-ri_TrigDesc-trig_update_before_row)
{
slot = ExecBRUpdateTriggers(estate, epqstate, resultRelInfo,
- tupleid, slot);
+ tupleid, slot, planSlot);
if (slot == NULL) /* do nothing */
return NULL;
@@ -737,6 +737,7 @@ lreplace:;
hufd.xmax);
if (!TupIsNull(epqslot))
{
+ planSlot = epqslot;
*tupleid = hufd.ctid;
slot = ExecFilterJunk(resultRelInfo-ri_junkFilter, epqslot);
tuple = ExecMaterializeSlot(slot);
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index
Re: [HACKERS] Parsing bool type value
On Tue, Aug 20, 2013 at 1:11 PM, Sawada Masahiko sawada.m...@gmail.com wrote: Hi all, Taking a look at PostgreSQL HEAD today, I noticed that currently PostgreSQL allows of value as bool type value. So user can execute the following SQL. =# SET enbale_seqscan TO of; And I read the source code related to parsing bool value. It compare TWO characters off and the setting value in parse_bool_with_len() function. Should we deny the of value as bool type value? When I checked the manual for values of bool types, it says as follows: Boolean values can be written as on, off, true, false, yes, no, 1, 0 (all case-insensitive) or any unambiguous prefix of these. Now of can be considered as unambiguous prefix of off, so it might be intentional. Please refer below link for more detailed description: http://www.postgresql.org/docs/devel/static/config-setting.html#CONFIG-SETTING-NAMES-VALUES With Regards, Amit Kapila. 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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
Stephen Frost escribió: * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Hopefully snippets put in conf.d/ by puppet/chef will override the settings in postgresql.conf (i.e. conf.d/ should be processed after postgresql.conf, not before); and hopefully ALTER SYSTEM will in turn override conf.d. I see no way to have ALTER SYSTEM handled by an include line, yet still have it override conf.d. With includedir and include directives, and postgresql.conf setting a defined ordering, with last-wins, you could simply have the 'includedir' for conf.d come before the 'include' for auto.conf. Uh, I was thinking that conf.d would be read by the system automatically, not because of an includedir line in postgresql.conf. -- Á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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Stephen Frost escribió: With includedir and include directives, and postgresql.conf setting a defined ordering, with last-wins, you could simply have the 'includedir' for conf.d come before the 'include' for auto.conf. Uh, I was thinking that conf.d would be read by the system automatically, not because of an includedir line in postgresql.conf. I'd much rather have an includedir directive than some hard-coded or command-line option to read the directory.. The directory should live in /etc/postgresql/X.Y/cluster/ on at least Debian derivatives.. Thanks, Stephen signature.asc Description: Digital signature
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
Stephen Frost escribió: * Amit Kapila (amit.kapil...@gmail.com) wrote: So let me try to explain what I understood from above: 1. enable_alter_system a new GUC whose default value =off. 2. Alter System will check this variable and return error (not allowed), if this parameter is off. 3. Now if user enables include directive in postgresql.conf, it will enable Alter System as value of enable_alter_system is on. 4. User can run Alter System command to disable Alter System enable_alter_system = off. Now even though include directive is enabled, but new Alter System commands will not work, however existing parameter's take into effect on restart/sighup. Yes. Not sure that it'd be terribly likely for a user to do that, but if they do it, so be it. With this design, if you put enable_alter_system=off in auto.conf, there is no way for the user to enable alter system again short of editing a file in the data directory. I think this is one of the things that was forbidden by policy; only files in the config directory needs to be edited. What I was proposing upthread is that enable_alter_system=off/on would be present in postgresql.conf, and there is no include line for auto.conf. That way, if the user wishes to enable/disable the feature, they need to edit postgresql.conf to do so. ALTER SYSTEM doesn't offer a way to disable itself. If ALTER SYSTEM is disabled via enable_alter_system=off in postgresql.conf, the settings in auto.conf are not read. -- Á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] Parsing bool type value
On Tue, Aug 20, 2013 at 11:53 PM, Amit Kapila amit.kapil...@gmail.com wrote: On Tue, Aug 20, 2013 at 1:11 PM, Sawada Masahiko sawada.m...@gmail.com wrote: Hi all, Taking a look at PostgreSQL HEAD today, I noticed that currently PostgreSQL allows of value as bool type value. So user can execute the following SQL. =# SET enbale_seqscan TO of; And I read the source code related to parsing bool value. It compare TWO characters off and the setting value in parse_bool_with_len() function. Should we deny the of value as bool type value? When I checked the manual for values of bool types, it says as follows: Boolean values can be written as on, off, true, false, yes, no, 1, 0 (all case-insensitive) or any unambiguous prefix of these. Now of can be considered as unambiguous prefix of off, so it might be intentional. Please refer below link for more detailed description: http://www.postgresql.org/docs/devel/static/config-setting.html#CONFIG-SETTING-NAMES-VALUES Thank you for replay. I have confirmed manual and understood. And I have understood the comment which is written at parse_bool_with_len() function. Thanks! Regards, --- Sawada Masahiko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
Stephen Frost escribió: * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Stephen Frost escribió: With includedir and include directives, and postgresql.conf setting a defined ordering, with last-wins, you could simply have the 'includedir' for conf.d come before the 'include' for auto.conf. Uh, I was thinking that conf.d would be read by the system automatically, not because of an includedir line in postgresql.conf. I'd much rather have an includedir directive than some hard-coded or command-line option to read the directory.. The directory should live in /etc/postgresql/X.Y/cluster/ on at least Debian derivatives.. The conf.d/ path would be relative to postgresql.conf, so there's no need for Debian to patch anything. -- Á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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
Alvaro Herrera alvhe...@2ndquadrant.com writes: What I was proposing upthread is that enable_alter_system=off/on would be present in postgresql.conf, and there is no include line for auto.conf. That way, if the user wishes to enable/disable the feature, they need to edit postgresql.conf to do so. ALTER SYSTEM doesn't offer a way to disable itself. If ALTER SYSTEM is disabled via enable_alter_system=off in postgresql.conf, the settings in auto.conf are not read. Personally, I'd get rid of any explicit enable_alter_system variable, and instead have an include directive for auto.conf. The functionality you describe above is then available by commenting or uncommenting the directive, plus the user has the option to decide where to put the directive (and thus control which settings can or can't be overridden). Anything else seems like it's going to be less flexible or else a lot more complicated. 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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: With this design, if you put enable_alter_system=off in auto.conf, there is no way for the user to enable alter system again short of editing a file in the data directory. I think this is one of the things that was forbidden by policy; only files in the config directory needs to be edited. If you edit it by hand to begin with (setting that parameter to 'off') then it's reasonable that you may have to edit it by hand again to fix it. If we don't want people to lock themselves out by using ALTER SYSTEM to turn it off, then we just disallow that. What I was proposing upthread is that enable_alter_system=off/on would be present in postgresql.conf, and there is no include line for auto.conf. I really think that's a terrible approach, to be honest. I want to see an 'include' line in postgresql.conf for auto.conf, so the hapless sysadmin who is trying to figure out what the crazy DBA did has some clue what to look for. enable_alter_system doesn't tell him diddly about an 'auto.conf' file which is included in the system config. That way, if the user wishes to enable/disable the feature, they need to edit postgresql.conf to do so. ALTER SYSTEM doesn't offer a way to disable itself. We can simply disallow ALTER SYSTEM from modifying enable_alter_system; that strikes me as a reasonable thing to do anyway. What I find a bit more worrying is what happens if they decide to put enable_alter_system=off into the postgresql.conf but keep the 'include' line for auto.conf.. Which goes right back to the question that I had before around if we want to complain when the same GUC is seen multiple times during parsing. It seems like there's no hope for it, given the way this has been designed, because you *must* set certain parameters and so you can't simply have them commented out, but those are likely to be parameters which DBAs will want to change through ALTER SYSTEM. Thanks, Stephen signature.asc Description: Digital signature
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Stephen Frost escribió: * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Stephen Frost escribió: With includedir and include directives, and postgresql.conf setting a defined ordering, with last-wins, you could simply have the 'includedir' for conf.d come before the 'include' for auto.conf. Uh, I was thinking that conf.d would be read by the system automatically, not because of an includedir line in postgresql.conf. I'd much rather have an includedir directive than some hard-coded or command-line option to read the directory.. The directory should live in /etc/postgresql/X.Y/cluster/ on at least Debian derivatives.. The conf.d/ path would be relative to postgresql.conf, so there's no need for Debian to patch anything. Uhhh, I really don't see that working, at all... Thanks, Stephen signature.asc Description: Digital signature
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
Stephen Frost sfr...@snowman.net writes: * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: The conf.d/ path would be relative to postgresql.conf, so there's no need for Debian to patch anything. Uhhh, I really don't see that working, at all... Why not? conf.d is for installable files, AIUI. What we need to be writable is auto.conf, but I thought we'd agreed that would live inside $PGDATA. 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] GSOC13 proposal - extend RETURNING syntax
W dniu 20.08.2013 16:47, Karol Trzcionka pisze:
Thank you for the review and tests. New version introduce a lot of
improvements:
- Fix regression test for view (wrong table_name)
- Add regression test for inheritance
- Delete hack in initsplan.c (now we ignore all RTE_BEFORE) - the
uninitialized issue
- Revert changing varno in add_vars_to_targetlist
- Add all before variables to targetlist
- Avoid adding variables to slot for AFTER.
- Treat varnoold like a flag - prevent from adjustment if RTE_BEFORE
- All before/after are now set on OUTER_VAR
- Rename fix_varno_varattno to bind_returning_variables
- Add comment about bind_returning_variables
- Remove unneeded code in fix_join_expr_mutator (it was changing varno
of RTE_BEFORE - now there is not any var with varno assigned to it)
I've just realized the prepare_returning_before() is unneeded right now
so I've removed it. Version 7, hopefully the last. ;)
Regards,
Karol Trzcionka
diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml
index 90b9208..eba35f0 100644
--- a/doc/src/sgml/ref/update.sgml
+++ b/doc/src/sgml/ref/update.sgml
@@ -194,12 +194,27 @@ UPDATE [ ONLY ] replaceable class=PARAMETERtable_name/replaceable [ * ] [
termreplaceable class=PARAMETERoutput_expression/replaceable/term
listitem
para
- An expression to be computed and returned by the commandUPDATE/
- command after each row is updated. The expression can use any
- column names of the table named by replaceable class=PARAMETERtable_name/replaceable
- or table(s) listed in literalFROM/.
- Write literal*/ to return all columns.
+ An expression to be computed and returned by the
+ commandUPDATE/ command either before or after (prefixed with
+ literalBEFORE./literal and literalAFTER./literal,
+ respectively) each row is updated. The expression can use any
+ column names of the table named by replaceable
+ class=PARAMETERtable_name/replaceable or table(s) listed in
+ literalFROM/. Write literalAFTER.*/literal to return all
+ columns after the update. Write literalBEFORE.*/literal for all
+ columns before the update. Write literal*/literal to return all
+ columns after update and all triggers fired (these values are in table
+ after command). You may combine BEFORE, AFTER and raw columns in the
+ expression.
/para
+ warningpara
+ Mixing table names or aliases named before or after with the
+ above will result in confusion and suffering. If you happen to
+ have a table called literalbefore/literal or
+ literalafter/literal, alias it to something else when using
+ RETURNING.
+ /para/warning
+
/listitem
/varlistentry
@@ -287,15 +302,16 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
/para
para
- Perform the same operation and return the updated entries:
+ Perform the same operation and return information on the changed entries:
programlisting
UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
WHERE city = 'San Francisco' AND date = '2003-07-03'
- RETURNING temp_lo, temp_hi, prcp;
+ RETURNING temp_lo AS new_low, temp_hi AS new_high, BEFORE.temp_hi/BEFORE.temp_low AS old_ratio, AFTER.temp_hi/AFTER.temp_low AS new_ratio prcp;
/programlisting
/para
+
para
Use the alternative column-list syntax to do the same update:
programlisting
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index d86e9ad..fafd311 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2335,7 +2335,7 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
TupleTableSlot *
ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
ResultRelInfo *relinfo,
- ItemPointer tupleid, TupleTableSlot *slot)
+ ItemPointer tupleid, TupleTableSlot *slot, TupleTableSlot **planSlot)
{
TriggerDesc *trigdesc = relinfo-ri_TrigDesc;
HeapTuple slottuple = ExecMaterializeSlot(slot);
@@ -2381,6 +2381,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
if (newSlot != NULL)
{
slot = ExecFilterJunk(relinfo-ri_junkFilter, newSlot);
+ *planSlot = newSlot;
slottuple = ExecMaterializeSlot(slot);
newtuple = slottuple;
}
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 15f5dcc..06ebaf3 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -603,7 +603,7 @@ ExecUpdate(ItemPointer tupleid,
resultRelInfo-ri_TrigDesc-trig_update_before_row)
{
slot = ExecBRUpdateTriggers(estate, epqstate, resultRelInfo,
- tupleid, slot);
+ tupleid, slot, planSlot);
if (slot == NULL) /* do nothing */
return NULL;
@@ -737,6 +737,7 @@ lreplace:;
hufd.xmax);
if (!TupIsNull(epqslot))
{
+ planSlot = epqslot;
*tupleid = hufd.ctid;
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
* Tom Lane (t...@sss.pgh.pa.us) wrote: Stephen Frost sfr...@snowman.net writes: * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: The conf.d/ path would be relative to postgresql.conf, so there's no need for Debian to patch anything. Uhhh, I really don't see that working, at all... Why not? conf.d is for installable files, AIUI. What we need to be writable is auto.conf, but I thought we'd agreed that would live inside $PGDATA. I agree that auto.conf should live in $PGDATA, but I really don't like the idea of conf.d being relative to some other existing file. It should be included through an 'includedir' option, not just picked up as some magic directory name, and therefore consider the current arrangement of parameters in Debian: data_directory = '/var/lib/postgresql/9.2/main' hba_file = '/etc/postgresql/9.2/main/pg_hba.conf' ident_file = '/etc/postgresql/9.2/main/pg_ident.conf' and postgres is started like so: /usr/lib/postgresql/9.2/bin/postgres -D /var/lib/postgresql/9.2/main -c config_file=/etc/postgresql/9.2/main/postgresql.conf With the proposed include line for auto.conf, which lives in $PGDATA, we'd have: include 'auto.conf' Would we then have includedir 'conf.d' which is relative to postgresql.conf instead? Thanks, Stephen signature.asc Description: Digital signature
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
Stephen Frost escribió: * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Stephen Frost escribió: I'd much rather have an includedir directive than some hard-coded or command-line option to read the directory.. The directory should live in /etc/postgresql/X.Y/cluster/ on at least Debian derivatives.. The conf.d/ path would be relative to postgresql.conf, so there's no need for Debian to patch anything. Uhhh, I really don't see that working, at all... I don't understand why not. Care to explain? -- Á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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Stephen Frost escribió: * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Stephen Frost escribió: I'd much rather have an includedir directive than some hard-coded or command-line option to read the directory.. The directory should live in /etc/postgresql/X.Y/cluster/ on at least Debian derivatives.. The conf.d/ path would be relative to postgresql.conf, so there's no need for Debian to patch anything. Uhhh, I really don't see that working, at all... I don't understand why not. Care to explain? Tried to in my other mail, but let me also point out that we (PGDG/Upstream) don't own the directory in which postgresql.conf lives. At least on Debian and relatives, that directory isn't under $PGDATA and it already has other files in it beyond postgresql.conf or even the other PostgreSQL config files of hba and ident. Here's the default directory setup on Debian for /etc/postgresql/9.2/main/: -rw-r--r-- 1 postgres postgres 316 Jun 29 22:07 environment -rw-r--r-- 1 postgres postgres 143 Jun 29 22:07 pg_ctl.conf -rw-r- 1 postgres postgres 4649 Jun 29 22:07 pg_hba.conf -rw-r- 1 postgres postgres 1636 Jun 29 22:07 pg_ident.conf -rw-r--r-- 1 postgres postgres 19770 Jun 29 22:07 postgresql.conf -rw-r--r-- 1 postgres postgres 378 Jun 29 22:07 start.conf There's three other files there and some sysadmins may have already created their own 'conf.d' directory, perhaps to use for building the postgresql.conf file or similar. We must have a way to disable the conf.d option and a way to name it something other than 'conf.d', imv. Thanks, Stephen signature.asc Description: Digital signature
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
Stephen Frost escribió: I agree that auto.conf should live in $PGDATA, but I really don't like the idea of conf.d being relative to some other existing file. It should be included through an 'includedir' option, not just picked up as some magic directory name, and therefore consider the current arrangement of parameters in Debian: data_directory = '/var/lib/postgresql/9.2/main' hba_file = '/etc/postgresql/9.2/main/pg_hba.conf' ident_file = '/etc/postgresql/9.2/main/pg_ident.conf' and postgres is started like so: /usr/lib/postgresql/9.2/bin/postgres -D /var/lib/postgresql/9.2/main -c config_file=/etc/postgresql/9.2/main/postgresql.conf With the proposed include line for auto.conf, which lives in $PGDATA, we'd have: include 'auto.conf' Would we then have includedir 'conf.d' which is relative to postgresql.conf instead? Well, all the relative paths in include/includedir directives would be relative to the directory specified by the -c config_file param, which makes perfect sense. So the conf.d would work fine in your example. The only problem I see in your snippet above is the include auto.conf line, which doesn't make any sense because that file would not be found. Hence my proposal that the file ought to be read automatically, not via an include line. Sadly I don't think we cannot just make it an absolute path, in case the data dir is moved or whatever; the only choice I see would be to have something like include-data 'auto.conf' or something like that which tells the system that the file is not in the config dir but in the data dir instead. A nearby comment could let the user know about this file being in the data directory instead of the config directory. -- Á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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
Stephen Frost escribió: Tried to in my other mail, Yep, got it and replied, thanks. but let me also point out that we (PGDG/Upstream) don't own the directory in which postgresql.conf lives. At least on Debian and relatives, that directory isn't under $PGDATA and it already has other files in it beyond postgresql.conf or even the other PostgreSQL config files of hba and ident. Here's the default directory setup on Debian for /etc/postgresql/9.2/main/: -rw-r--r-- 1 postgres postgres 316 Jun 29 22:07 environment -rw-r--r-- 1 postgres postgres 143 Jun 29 22:07 pg_ctl.conf -rw-r- 1 postgres postgres 4649 Jun 29 22:07 pg_hba.conf -rw-r- 1 postgres postgres 1636 Jun 29 22:07 pg_ident.conf -rw-r--r-- 1 postgres postgres 19770 Jun 29 22:07 postgresql.conf -rw-r--r-- 1 postgres postgres 378 Jun 29 22:07 start.conf There's three other files there and some sysadmins may have already created their own 'conf.d' directory, perhaps to use for building the postgresql.conf file or similar. We must have a way to disable the conf.d option and a way to name it something other than 'conf.d', imv. Uhm. I find it hard to care much for this position. Surely config files are not migrated automatically from one major version to the next, so if somebody created a 9.3/main/conf.d directory, they will have to change it when they migrate to 9.4. -- Á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
[HACKERS] CAST Within EXCLUSION constraint
Hackers, I am trying to do something like this: CREATE TYPE source AS ENUM( 'fred', 'wilma', 'barney', 'betty' ); CREATE EXTENSION btree_gist; CREATE TABLE things ( source source NOT NULL, within tstzrange NOT NULL, EXCLUDE USING gist (source WITH =, within WITH ) ); Alas, enums are not supported by btree_gist: try.sql:13: ERROR: data type source has no default operator class for access method gist HINT: You must specify an operator class for the index or define a default operator class for the data type. Well, maybe I can cast it? But no, changing the EXCLUDE line to EXCLUDE USING gist (source::text WITH =, within WITH ) Yields a syntax error: try.sql:13: ERROR: syntax error at or near :: LINE 4: EXCLUDE USING gist (source::text WITH =, within WITH ) So that's out. Why shouldn't :: be allowed? No problem, I can use CAST(), right? So I try: EXCLUDE USING gist (CAST(source AS text) WITH =, within WITH ) Not so much: try.sql:13: ERROR: functions in index expression must be marked IMMUTABLE I guess it's because locale settings might change, and therefore change the text representation? Seems unlikely, though. I guess I can create my own IMMUTABLE function over the ENUM: CREATE FUNCTION source_to_text( source ) RETURNS TEXT LANGUAGE sql STRICT IMMUTABLE AS $$ SELECT $1::text; $$; So this works: EXCLUDE USING gist (source_to_text(source) WITH =, within WITH ) So I guess that’s good enough for now. But should :: really be a syntax error in index expressions? Thanks, David -- 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] ereport documentation patch
On Aug 19, 2013, at 11:28 PM, Heikki Linnakangas wrote: On 19.08.2013 23:40, Christophe Pettus wrote: Is it reasonable to note in the documentation that ereport does not return if the error severity is greater than or equal to ERROR? Yeah, it probably would be good to mention that. Got a patch? Attached! ereport-no-return-doc.patch Description: Binary data -- -- Christophe Pettus x...@thebuild.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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Well, all the relative paths in include/includedir directives would be relative to the directory specified by the -c config_file param, which makes perfect sense. So the conf.d would work fine in your example. Why would include/includedir directives be relative to where the 'config_file' GUC is set to instead of relative to where all the other GUCs in postgresql.conf are relative to? That is a recipe for confusion, imv. Of course, the current situation is quite terrible anyway, imv. Having the GUCs be relative to whereever the user happens to run pg_ctl from is pretty ugly- not to mention that the commented out 'defaults' won't actually work if you uncomment them because the *actual* default/unset value *is* in the data directory. I'm starting to wish for a way to do variable substitution inside postgresql.conf, so we could have defaults that would actually work if uncommented (eg: $DataDir/pg_hba.conf). That would help with auto.conf also. The only problem I see in your snippet above is the include auto.conf line, which doesn't make any sense because that file would not be found. To be honest, I was considering 'include' to be relative to the data directory and handled similar to how we process recovery.conf, but as we consider paths in postgresql.conf to be relative to $PWD, that's not ideal because it'd be different from the rest of the file references. In any case, while the current situation sucks, I don't think we can go changing how relative files are treated in postgresql.conf, nor should we make the way a path is processed change depending on which GUC is being set. While I really like the 'include auto.conf' style, I'm starting to think it may not be workable after all. Another thing to consider is if the user decides to change that include line.. What happens when the DBA tries to do a 'ALTER SYSTEM'? It'd still use the hard-coded auto.conf file and happily update it, I imagine, but it won't actually get included... Thanks, Stephen signature.asc Description: Digital signature
[HACKERS] Detail part for still waiting for lock log message
Hello!
From time to time when investigating different locking issues using
postgres log i have thought that process x is still waiting for
message could be more informative, for example at the moment it is quite
painful to find out which session was actually holding that particular lock.
I would like to add detail part for this message to show information
about the lock holder and also show what the lock holder is actually
doing (yes, i know that i could get the lock holder's statement from the
log, but this not the same, maybe lock holder was idle in transaction).
So, i wrote a small patch, but i am not sure that this is the
best/correct way to do it.
Maybe someone can take a look at my patch and give some feedback.
Even if this idea won't reach to upstream, i would still like to get
feedback.
About patch:
Patch is tested against 9.2.4.
I was not sure that i should check if the lock holder's proclock was
found (as lock holder's proclock should be always there), check is there
to be on the safe side, but maybe it's unnecessary.
If it's not needed then fallback to old behavior (logging without
detail) is not needed as well.
And yes, i know that the lock holding time is not actually correct and
it actually shows milliseconds since transaction start.
Regards,
Tarvi Pillessaar
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 3bfdd23..4dc7b37 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -49,6 +49,7 @@
#include storage/procsignal.h
#include storage/spin.h
#include utils/timestamp.h
+#include pgstat.h
/* GUC variables */
@@ -1198,9 +1199,66 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
}
if (myWaitStatus == STATUS_WAITING)
-ereport(LOG,
- (errmsg(process %d still waiting for %s on %s after %ld.%03d ms,
- MyProcPid, modename, buf.data, msecs, usecs)));
+ {
+PROCLOCK *proclock2;
+PGPROC *proc2;
+PgBackendStatus *be;
+bool found = false;
+
+/* find lock holder */
+proclock2 = (PROCLOCK *) SHMQueueNext((lock-procLocks), (lock-procLocks),
+ offsetof(PROCLOCK, lockLink));
+while (proclock2)
+{
+ if (lockMethodTable-conflictTab[lockmode] proclock2-holdMask)
+ break;
+
+ proclock2 = (PROCLOCK *) SHMQueueNext((lock-procLocks), proclock2-lockLink,
+ offsetof(PROCLOCK, lockLink));
+}
+
+if (proclock2)
+{
+ proc2 = proclock2-tag.myProc;
+
+ /* get lock holder's beentry */
+ int numbackends = pgstat_fetch_stat_numbackends();
+ for (i = 1; i = numbackends; i++)
+ {
+ be = pgstat_fetch_stat_beentry(i);
+ if (be)
+ {
+ if (be-st_procpid == proc2-pid)
+ {
+found = true;
+break;
+ }
+ }
+ }
+}
+
+if (found)
+{
+ long secs2;
+ int usecs2;
+ long msecs2;
+
+ /* calculate lock holder's tx duration */
+ TimestampDifference(be-st_xact_start_timestamp, GetCurrentTimestamp(), secs2, usecs2);
+ msecs2 = secs2 * 1000 + usecs2 / 1000;
+ usecs2 = usecs2 % 1000;
+
+ ereport(LOG,
+ (errmsg(process %d still waiting for %s on %s after %ld.%03d ms,
+ MyProcPid, modename, buf.data, msecs, usecs),
+ errdetail_log(process %d is holding lock for %ld.%03d ms, activity: %s,
+ proc2-pid, msecs2, usecs2, be-st_activity)));
+}
+else
+ ereport(LOG,
+ (errmsg(process %d still waiting for %s on %s after %ld.%03d ms,
+ MyProcPid, modename, buf.data, msecs, usecs)));
+ }
else if (myWaitStatus == STATUS_OK)
ereport(LOG,
(errmsg(process %d acquired %s on %s after %ld.%03d ms,
--
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] Personal note: taking some vacation time in Sep/Oct
On 08/19/2013 11:55 PM, Gavin Flower wrote: On 20/08/13 15:26, Tom Lane wrote: I will be taking a long (and long-overdue) vacation... but, But, BUT, you're not human - you can't possibly take leave, the sky will fall all manners of divers calamities will come to pass!!! As if on cue: http://www.nasa.gov/content/goddard/the-suns-magnetic-field-is-about-to-flip/ Cheers, Steve
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
Stephen Frost escribió: * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Well, all the relative paths in include/includedir directives would be relative to the directory specified by the -c config_file param, which makes perfect sense. So the conf.d would work fine in your example. Why would include/includedir directives be relative to where the 'config_file' GUC is set to instead of relative to where all the other GUCs in postgresql.conf are relative to? That is a recipe for confusion, imv. Of course, the current situation is quite terrible anyway, imv. Having the GUCs be relative to whereever the user happens to run pg_ctl from is pretty ugly- not to mention that the commented out 'defaults' won't actually work if you uncomment them because the *actual* default/unset value *is* in the data directory. Uh? See the docs: http://www.postgresql.org/docs/devel/static/config-setting.html#CONFIG-INCLUDES ... the postgresql.conf file can contain include directives, ... If the file name is not an absolute path, it is taken as relative to the directory containing the referencing configuration file. I'm starting to wish for a way to do variable substitution inside postgresql.conf, so we could have defaults that would actually work if uncommented (eg: $DataDir/pg_hba.conf). That would help with auto.conf also. Grumble. I don't think we need this. At least, not for ALTER SYSTEM or conf.d. The only problem I see in your snippet above is the include auto.conf line, which doesn't make any sense because that file would not be found. To be honest, I was considering 'include' to be relative to the data directory and handled similar to how we process recovery.conf, Well, recovery.conf is a special case that doesn't follow to normal rules. but as we consider paths in postgresql.conf to be relative to $PWD, that's not ideal because it'd be different from the rest of the file references. I don't know where you got that idea from, but it's wrong. While I really like the 'include auto.conf' style, I'm starting to think it may not be workable after all. Another thing to consider is if the user decides to change that include line.. What happens when the DBA tries to do a 'ALTER SYSTEM'? It'd still use the hard-coded auto.conf file and happily update it, I imagine, but it won't actually get included... Well, this whole line of discussion started because I objected to the whole code path that was trying to detect whether auto.conf had been parsed, and raised a warning if ALTER SYSTEM was executed and the file wasn't parsed. -- Á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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
Alvaro Herrera alvhe...@2ndquadrant.com writes: Stephen Frost escribió: While I really like the 'include auto.conf' style, I'm starting to think it may not be workable after all. Another thing to consider is if the user decides to change that include line.. What happens when the DBA tries to do a 'ALTER SYSTEM'? It'd still use the hard-coded auto.conf file and happily update it, I imagine, but it won't actually get included... Well, this whole line of discussion started because I objected to the whole code path that was trying to detect whether auto.conf had been parsed, and raised a warning if ALTER SYSTEM was executed and the file wasn't parsed. I really, really don't think that we should be trying to detect or prevent any such thing. If the user breaks it like that, he gets to keep both pieces --- and who's to say it's broken, anyway? Disabling ALTER SYSTEM might have been exactly his intent. 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] Personal note: taking some vacation time in Sep/Oct
On Tue, Aug 20, 2013 at 2:55 AM, Gavin Flower gavinflo...@archidevsys.co.nz wrote: On 20/08/13 15:26, Tom Lane wrote: I will be taking a long (and long-overdue) vacation from Sep 10 to Oct 20. I expect to have email access, but won't be doing much more than minimally keeping up with my inbox. This means I'll be pretty much AWOL for the September commitfest :-(. That's unfortunate, but the dates for this trip were frozen long before the 9.4 development calendar was. regards, tom lane but, But, BUT, you're not human - you can't possibly take leave, the sky will fall all manners of divers calamities will come to pass!!! I think a scene from Ghostbusters comes in handy here... Dr. Peter Venkman: This city is headed for a disaster of biblical proportions. Mayor: What do you mean, biblical? Dr Ray Stantz: What he means is Old Testament, Mr. Mayor, real wrath of God type stuff. Dr. Peter Venkman: Exactly. Dr Ray Stantz: Fire and brimstone coming down from the skies! Rivers and seas boiling! Dr. Egon Spengler: Forty years of darkness! Earthquakes, volcanoes... Winston Zeddemore: The dead rising from the grave! Dr. Peter Venkman: Human sacrifice, dogs and cats living together... mass hysteria! Mayor: All right, all right! I get the point! Man, dogs and cats living together!!! [I wonder what skewing this will have on the analytical statistics on the pgsql mailing lists??? http://www.citusdata.com/blog/57-postgresql-full-text-search] MORE SERIOUSLY: Enjoy your more than well earnt leave! Indeed. We'll try to keep the dogs and cats suitably apart! :-) -- When confronted by a difficult problem, solve it by reducing it to the question, How would the Lone Ranger handle this? -- 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] CAST Within EXCLUSION constraint
David E. Wheeler da...@justatheory.com writes: Well, maybe I can cast it? But no, changing the EXCLUDE line to EXCLUDE USING gist (source::text WITH =, within WITH ) Yields a syntax error: try.sql:13: ERROR: syntax error at or near :: LINE 4: EXCLUDE USING gist (source::text WITH =, within WITH ) So that's out. Why shouldn't :: be allowed? You need more parentheses -- (source::text) would've worked. No problem, I can use CAST(), right? So I try: EXCLUDE USING gist (CAST(source AS text) WITH =, within WITH ) Not so much: try.sql:13: ERROR: functions in index expression must be marked IMMUTABLE I guess it's because locale settings might change, and therefore change the text representation? Seems unlikely, though. Not locale, just renaming one of the values would be enough to break that. Admittedly we don't provide an official way to do that ATM, but you can do an UPDATE on pg_enum. 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] CAST Within EXCLUSION constraint
On Aug 20, 2013, at 6:50 PM, Tom Lane t...@sss.pgh.pa.us wrote: You need more parentheses -- (source::text) would've worked. Alas, no, same problem as for CAST(): ERROR: functions in index expression must be marked IMMUTABLE No problem, I can use CAST(), right? So I try: EXCLUDE USING gist (CAST(source AS text) WITH =, within WITH ) Not so much: try.sql:13: ERROR: functions in index expression must be marked IMMUTABLE I guess it's because locale settings might change, and therefore change the text representation? Seems unlikely, though. Not locale, just renaming one of the values would be enough to break that. Admittedly we don't provide an official way to do that ATM, but you can do an UPDATE on pg_enum. Ah, right. Maybe if there was a way to get at some immutable numeric value… Thanks, David -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Stephen Frost escribió: Of course, the current situation is quite terrible anyway, imv. Having the GUCs be relative to whereever the user happens to run pg_ctl from is pretty ugly- not to mention that the commented out 'defaults' won't actually work if you uncomment them because the *actual* default/unset value *is* in the data directory. Uh? See the docs: http://www.postgresql.org/docs/devel/static/config-setting.html#CONFIG-INCLUDES ... the postgresql.conf file can contain include directives, ... If the file name is not an absolute path, it is taken as relative to the directory containing the referencing configuration file. And what about http://www.postgresql.org/docs/9.3/static/runtime-config-file-locations.html ... ? When setting any of these parameters, a relative path will be interpreted with respect to the directory in which postgres is started. I'm starting to wish for a way to do variable substitution inside postgresql.conf, so we could have defaults that would actually work if uncommented (eg: $DataDir/pg_hba.conf). That would help with auto.conf also. Grumble. I don't think we need this. At least, not for ALTER SYSTEM or conf.d. imv, it would be handy. Along with a $ConfDir which is the postgresql.conf location- it would certainly make things clearer about what files are being included from where. To be honest, I was considering 'include' to be relative to the data directory and handled similar to how we process recovery.conf, Well, recovery.conf is a special case that doesn't follow to normal rules. I don't see why it should be. but as we consider paths in postgresql.conf to be relative to $PWD, that's not ideal because it'd be different from the rest of the file references. I don't know where you got that idea from, but it's wrong. Not sure which you're referring to, but see above from the docs? Also, please tias.. I was amazed that we use $PWD also, but we do.. Well, this whole line of discussion started because I objected to the whole code path that was trying to detect whether auto.conf had been parsed, and raised a warning if ALTER SYSTEM was executed and the file wasn't parsed. I like the idea that we complain if ALTER SYSTEM ends up being idempotent.. Not sure how far we should take that, but accepting commands which end up doing nothing is bad, imv. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Detail part for still waiting for lock log message
Tarvi Pillessaar tar...@gmail.com wrote: Maybe someone can take a look at my patch and give some feedback. Even if this idea won't reach to upstream, i would still like to get feedback. Please add the patch to the open CommitFest. You can read about the process here: http://wiki.postgresql.org/wiki/CommitFest -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
Stephen Frost escribió: And what about http://www.postgresql.org/docs/9.3/static/runtime-config-file-locations.html ... ? When setting any of these parameters, a relative path will be interpreted with respect to the directory in which postgres is started. That's talking about the data_directory and the various foo_file settings, though; it doesn't apply to the include settings. Note especially that config_file says it can only be set on the postgres command line. (I think a saner definition would have been to state that relative paths are not allowed in the command line. But that ship has already sailed. And relative paths seem useful in the config file; and maintaining the distinction that they are allowed within the config file but not in the command line might be awkward.) (Uhm, when a command line contains stuff, is the stuff in the command line or on it?) -- Á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] GSOC13 proposal - extend RETURNING syntax
2013-08-20 17:30 keltezéssel, Karol Trzcionka írta: W dniu 20.08.2013 16:47, Karol Trzcionka pisze: Thank you for the review and tests. New version introduce a lot of improvements: - Fix regression test for view (wrong table_name) - Add regression test for inheritance - Delete hack in initsplan.c (now we ignore all RTE_BEFORE) - the uninitialized issue - Revert changing varno in add_vars_to_targetlist - Add all before variables to targetlist - Avoid adding variables to slot for AFTER. - Treat varnoold like a flag - prevent from adjustment if RTE_BEFORE - All before/after are now set on OUTER_VAR - Rename fix_varno_varattno to bind_returning_variables - Add comment about bind_returning_variables - Remove unneeded code in fix_join_expr_mutator (it was changing varno of RTE_BEFORE - now there is not any var with varno assigned to it) I've just realized the prepare_returning_before() is unneeded right now so I've removed it. Version 7, hopefully the last. ;) Here's a new one, for v7: setrefs.c: In function ‘set_plan_refs’: setrefs.c:2001:26: warning: ‘before_index’ may be used uninitialized in this function [-Wmaybe-uninitialized] bind_returning_variables(rlist, before_index, after_index); ^ setrefs.c:1957:21: note: ‘before_index’ was declared here int after_index=0, before_index; ^ Best regards, Zoltán Böszörményi Regards, Karol Trzcionka -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
W dniu 20.08.2013 20:55, Boszormenyi Zoltan pisze:
Here's a new one, for v7:
setrefs.c: In function ‘set_plan_refs’:
setrefs.c:2001:26: warning: ‘before_index’ may be used uninitialized
in this function [-Wmaybe-uninitialized]
bind_returning_variables(rlist, before_index, after_index);
^
setrefs.c:1957:21: note: ‘before_index’ was declared here
int after_index=0, before_index;
^
Right, my mistake. Sorry and thanks. Fixed.
Regards,
Karol Trzcionka
diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml
index 90b9208..eba35f0 100644
--- a/doc/src/sgml/ref/update.sgml
+++ b/doc/src/sgml/ref/update.sgml
@@ -194,12 +194,27 @@ UPDATE [ ONLY ] replaceable class=PARAMETERtable_name/replaceable [ * ] [
termreplaceable class=PARAMETERoutput_expression/replaceable/term
listitem
para
- An expression to be computed and returned by the commandUPDATE/
- command after each row is updated. The expression can use any
- column names of the table named by replaceable class=PARAMETERtable_name/replaceable
- or table(s) listed in literalFROM/.
- Write literal*/ to return all columns.
+ An expression to be computed and returned by the
+ commandUPDATE/ command either before or after (prefixed with
+ literalBEFORE./literal and literalAFTER./literal,
+ respectively) each row is updated. The expression can use any
+ column names of the table named by replaceable
+ class=PARAMETERtable_name/replaceable or table(s) listed in
+ literalFROM/. Write literalAFTER.*/literal to return all
+ columns after the update. Write literalBEFORE.*/literal for all
+ columns before the update. Write literal*/literal to return all
+ columns after update and all triggers fired (these values are in table
+ after command). You may combine BEFORE, AFTER and raw columns in the
+ expression.
/para
+ warningpara
+ Mixing table names or aliases named before or after with the
+ above will result in confusion and suffering. If you happen to
+ have a table called literalbefore/literal or
+ literalafter/literal, alias it to something else when using
+ RETURNING.
+ /para/warning
+
/listitem
/varlistentry
@@ -287,15 +302,16 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
/para
para
- Perform the same operation and return the updated entries:
+ Perform the same operation and return information on the changed entries:
programlisting
UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
WHERE city = 'San Francisco' AND date = '2003-07-03'
- RETURNING temp_lo, temp_hi, prcp;
+ RETURNING temp_lo AS new_low, temp_hi AS new_high, BEFORE.temp_hi/BEFORE.temp_low AS old_ratio, AFTER.temp_hi/AFTER.temp_low AS new_ratio prcp;
/programlisting
/para
+
para
Use the alternative column-list syntax to do the same update:
programlisting
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index d86e9ad..fafd311 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2335,7 +2335,7 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
TupleTableSlot *
ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
ResultRelInfo *relinfo,
- ItemPointer tupleid, TupleTableSlot *slot)
+ ItemPointer tupleid, TupleTableSlot *slot, TupleTableSlot **planSlot)
{
TriggerDesc *trigdesc = relinfo-ri_TrigDesc;
HeapTuple slottuple = ExecMaterializeSlot(slot);
@@ -2381,6 +2381,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
if (newSlot != NULL)
{
slot = ExecFilterJunk(relinfo-ri_junkFilter, newSlot);
+ *planSlot = newSlot;
slottuple = ExecMaterializeSlot(slot);
newtuple = slottuple;
}
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 15f5dcc..06ebaf3 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -603,7 +603,7 @@ ExecUpdate(ItemPointer tupleid,
resultRelInfo-ri_TrigDesc-trig_update_before_row)
{
slot = ExecBRUpdateTriggers(estate, epqstate, resultRelInfo,
- tupleid, slot);
+ tupleid, slot, planSlot);
if (slot == NULL) /* do nothing */
return NULL;
@@ -737,6 +737,7 @@ lreplace:;
hufd.xmax);
if (!TupIsNull(epqslot))
{
+ planSlot = epqslot;
*tupleid = hufd.ctid;
slot = ExecFilterJunk(resultRelInfo-ri_junkFilter, epqslot);
tuple = ExecMaterializeSlot(slot);
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 908f397..461ec4f 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -1987,6 +1987,7 @@ range_table_walker(List *rtable,
{
case RTE_RELATION:
case RTE_CTE:
+ case RTE_BEFORE:
/* nothing to do */
break;
case
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Stephen Frost escribió: http://www.postgresql.org/docs/9.3/static/runtime-config-file-locations.html That's talking about the data_directory and the various foo_file settings, though; it doesn't apply to the include settings. Right- that's what I'm bitching about. We have various references to file locations, with defined handling of relative locations, and the 'include' system completely ignores that and instead invents its own making the result a confusing mess. Note especially that config_file says it can only be set on the postgres command line. (I think a saner definition would have been to state that relative paths are not allowed in the command line. But that ship has already sailed. And relative paths seem useful in the config file; and maintaining the distinction that they are allowed within the config file but not in the command line might be awkward.) Relative paths based on $PWD are useful? Really? Not on my systems anyway.. (Uhm, when a command line contains stuff, is the stuff in the command line or on it?) A just question- I vote 'on'. :) Thanks, Stephen signature.asc Description: Digital signature
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
On Tue, Aug 20, 2013 at 11:23:06AM -0400, Stephen Frost wrote: What I was proposing upthread is that enable_alter_system=off/on would be present in postgresql.conf, and there is no include line for auto.conf. I really think that's a terrible approach, to be honest. I want to see an 'include' line in postgresql.conf for auto.conf, so the hapless sysadmin who is trying to figure out what the crazy DBA did has some clue what to look for. enable_alter_system doesn't tell him diddly about an 'auto.conf' file which is included in the system config. ISTM you want some kind of hybrid setting like: #include_system auto.conf which simultaneously does three things: 1. Sets the enable_alter_system flag 2. Indicates the file to use 3. Indicates the priority of the setting re other settings. Comment it out, ALTER SYSTEM stop working. Put it back and it's immediately clear what it means. And the user can control where the settings go. Syntax is a bit fugly though. Have a nice day, -- Martijn van Oosterhout klep...@svana.org http://svana.org/kleptog/ He who writes carelessly confesses thereby at the very outset that he does not attach much importance to his own thoughts. -- Arthur Schopenhauer signature.asc Description: Digital signature
[HACKERS] Back-patch change in hashed DISTINCT estimation?
In a thread over in pgsql-performance, Tomas Vondra pointed out that choose_hashed_distinct was sometimes making different choices than choose_hashed_grouping, so that queries like these: select distinct x from ... ; select x from ... group by 1; might get different plans even though they should be equivalent. After some debugging it turns out that I omitted hash_agg_entry_size() from the hash table size estimate in choose_hashed_distinct --- I'm pretty sure I momentarily thought that this function must yield zero if there are no aggregates, but that's wrong. So we need a patch like this: diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index bcc0d45..99284cb 100644 *** a/src/backend/optimizer/plan/planner.c --- b/src/backend/optimizer/plan/planner.c *** choose_hashed_distinct(PlannerInfo *root *** 2848,2854 --- 2848,2858 * Don't do it if it doesn't look like the hashtable will fit into * work_mem. */ + + /* Estimate per-hash-entry space at tuple width... */ hashentrysize = MAXALIGN(path_width) + MAXALIGN(sizeof(MinimalTupleData)); + /* plus the per-hash-entry overhead */ + hashentrysize += hash_agg_entry_size(0); if (hashentrysize * dNumDistinctRows work_mem * 1024L) return false; When grouping narrow data, like a float or a couple of ints, this oversight makes for more than 2X error in the hash table size estimate. What I'm wondering is whether to back-patch this or leave well enough alone. The risk of back-patching is that it might destabilize plan choices that people like. (In Tomas' original example, the underestimate of the table size leads it to choose a plan that is in fact better.) The risk of not back-patching is that the error could lead to out-of-memory failures because the hash aggregation uses more memory than the planner expected. (Tomas was rather fortunate in that his case had an overestimate of dNumDistinctRows, so it didn't end up blowing out memory ... but usually I think we underestimate that more than overestimate it.) A possible compromise is to back-patch into 9.3 (where the argument about destabilizing plan choices doesn't have much force yet), but not further. Thoughts? 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] danger of stats_temp_directory = /dev/shm
Tom Lane wrote:
Andres Freund and...@2ndquadrant.com writes:
On 2013-08-19 14:28:28 -0400, Tom Lane wrote:
One possibility is to do the initial check somewhere shortly after
ChangeToDataDir(), and have the GUC check hook only attempt to make a
check in SIGHUP context. Unfortunately we aren't passing the context to
check hooks, only GucSource which isn't adequate for this. Not sure if we
want to go so far as to change the check-hook API at this point. We could
probably think of some other, klugy way to tell if it's initial startup.
Is it even actually safe to have stats_temp_directory PGC_SIGHUP after
the per DB splitup? I haven't audited the code for it, but it seems
somewhat likely that we would end up with some files in the old and some
in the new directory?
That's a good point. For the moment we could just change it to
PGC_POSTMASTER and eliminate this problem. Reducing it back to SIGHUP
would be a future feature, which is evidently less than trivial.
Here's a patchset for this. The first patch is the same as upthread,
with the enum members renamed; the second is the actual pgstats change.
(I considered the idea that checkDirectoryPermissions returned a bitmask
of the failed checks, instead of just returning a code for the first
check that fails. This might be useful if some caller wants to ignore
certain problems. But at the moment I didn't see many other places
where this could be used, so it's probably pointless ATM.)
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
***
*** 1313,1385 getInstallationPaths(const char *argv0)
static void
checkDataDir(void)
{
char path[MAXPGPATH];
FILE *fp;
- struct stat stat_buf;
Assert(DataDir);
! if (stat(DataDir, stat_buf) != 0)
{
! if (errno == ENOENT)
ereport(FATAL,
(errcode_for_file_access(),
errmsg(data directory \%s\ does not exist,
DataDir)));
! else
ereport(FATAL,
(errcode_for_file_access(),
! errmsg(could not read permissions of directory \%s\: %m,
! DataDir)));
}
- /* eventual chdir would fail anyway, but let's test ... */
- if (!S_ISDIR(stat_buf.st_mode))
- ereport(FATAL,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg(specified data directory \%s\ is not a directory,
- DataDir)));
-
- /*
- * Check that the directory belongs to my userid; if not, reject.
- *
- * This check is an essential part of the interlock that prevents two
- * postmasters from starting in the same directory (see CreateLockFile()).
- * Do not remove or weaken it.
- *
- * XXX can we safely enable this check on Windows?
- */
- #if !defined(WIN32) !defined(__CYGWIN__)
- if (stat_buf.st_uid != geteuid())
- ereport(FATAL,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg(data directory \%s\ has wrong ownership,
- DataDir),
- errhint(The server must be started by the user that owns the data directory.)));
- #endif
-
- /*
- * Check if the directory has group or world access. If so, reject.
- *
- * It would be possible to allow weaker constraints (for example, allow
- * group access) but we cannot make a general assumption that that is
- * okay; for example there are platforms where nearly all users
- * customarily belong to the same group. Perhaps this test should be
- * configurable.
- *
- * XXX temporarily suppress check when on Windows, because there may not
- * be proper support for Unix-y file permissions. Need to think of a
- * reasonable check to apply on Windows.
- */
- #if !defined(WIN32) !defined(__CYGWIN__)
- if (stat_buf.st_mode (S_IRWXG | S_IRWXO))
- ereport(FATAL,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg(data directory \%s\ has group or world access,
- DataDir),
- errdetail(Permissions should be u=rwx (0700).)));
- #endif
-
/* Look for PG_VERSION before looking for pg_control */
ValidatePgVersion(DataDir);
--- 1313,1363
static void
checkDataDir(void)
{
+ CheckDirErrcode err;
char path[MAXPGPATH];
FILE *fp;
Assert(DataDir);
! err = checkDirectoryPermissions(DataDir);
! switch (err)
{
! case CKDIR_OK:
! break;
! case CKDIR_NOT_EXISTS:
ereport(FATAL,
(errcode_for_file_access(),
errmsg(data directory \%s\ does not exist,
DataDir)));
! break;
! case CKDIR_CANT_READ_PERMS:
ereport(FATAL,
(errcode_for_file_access(),
! errmsg(could not read permissions of data directory \%s\: %m,
! DataDir)));
! break;
! case CKDIR_NOT_DIR:
! ereport(FATAL,
! (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
! errmsg(specified data directory \%s\ is not a directory,
! DataDir)));
! break;
! case
Re: [HACKERS] 9.4 regression
On Mon, Aug 19, 2013 at 09:27:57PM -0400, Stephen Frost wrote: * Andres Freund (and...@2ndquadrant.com) wrote: I vote for adapting the patch to additionally zero out the file via write(). In your tests that seemed to perform at least as good as the old method... It also has the advantage that we can use it a littlebit more as a testbed for possibly using it for heap extensions one day. We're pretty early in the cycle, so I am not worried about this too much... I dunno, I'm pretty disappointed that this doesn't actually improve things. Just following this casually, it looks like it might be some kind of locking issue in the kernel that's causing it to be slower; or at least some code path that isn't exercise terribly much and therefore hasn't been given the love that it should. Definitely interested in what Ts'o says, but if we can't figure out why it's slower *without* writing out the zeros, I'd say we punt on this until Linux and the other OS folks improve the situation. Agreed. Anyone with an affected kernel really can't be doing performance tests right now, and that isn't good. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
On Tue, Aug 20, 2013 at 08:38:52AM +0530, Amit Kapila wrote: On Tue, Aug 20, 2013 at 8:27 AM, Stephen Frost sfr...@snowman.net wrote: * Amit Kapila (amit.kapil...@gmail.com) wrote: If both are okay, then I would like to go with second option (include file mechanism). I think by default, we should allow auto file to be included and if user faces any problem or otherwise, then he can decide to disable it. If it's enabled by default, then we need to ship an 'auto' file which is empty by default... initdb will create the empty auto file. If we don't enable by default, then if user uses ALTER SYSTEM and do sighup/restart, changed config parameter values will not come into affect until he manually enables it by removing '#' from '#include'. Just a heads-up, but a lot of C language folks are going to look at: #include abc and think that is enabled. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] Back-patch change in hashed DISTINCT estimation?
On Wed, Aug 21, 2013 at 2:54 AM, Tom Lane t...@sss.pgh.pa.us wrote: What I'm wondering is whether to back-patch this or leave well enough alone. The risk of back-patching is that it might destabilize plan choices that people like. (In Tomas' original example, the underestimate of the table size leads it to choose a plan that is in fact better.) The risk of not back-patching is that the error could lead to out-of-memory failures because the hash aggregation uses more memory than the planner expected. FWIW I recently investigated an out-of-memory issue in hash aggregation. That case was because of use of a large temp table which was not manually analysed and thus lead to a bad plan selection. But out of memory errors are very confusing to the users and I have seen them unnecessarily tinkering their memory settings to circumvent that issue. So +1 to fix the bug in back branches, even though I understand there could be some casualties on the border. Thanks, Pavan -- Pavan Deolasee http://www.linkedin.com/in/pavandeolasee
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
On Tue, Aug 20, 2013 at 10:02 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Stephen Frost escribió: * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Well, all the relative paths in include/includedir directives would be relative to the directory specified by the -c config_file param, which makes perfect sense. So the conf.d would work fine in your example. Why would include/includedir directives be relative to where the 'config_file' GUC is set to instead of relative to where all the other GUCs in postgresql.conf are relative to? That is a recipe for confusion, imv. Of course, the current situation is quite terrible anyway, imv. Having the GUCs be relative to whereever the user happens to run pg_ctl from is pretty ugly- not to mention that the commented out 'defaults' won't actually work if you uncomment them because the *actual* default/unset value *is* in the data directory. Uh? See the docs: http://www.postgresql.org/docs/devel/static/config-setting.html#CONFIG-INCLUDES ... the postgresql.conf file can contain include directives, ... If the file name is not an absolute path, it is taken as relative to the directory containing the referencing configuration file. You are right and I have checked code as well, it works in above way for include directives. Now the question I have in mind is that even if we can't directly use include directive to enable/disable Alter System and reading of auto file, how would a new GUC enable_alter_system can solve all the things. It can allow/disallow Alter System command, but how about reading of auto file? If we directly read auto file without considering enable_alter_system, it can lead to chaos due to some un-safe values, on the other side if we want to consider enable_alter_system before reading file, it can complicate the ProcessConfigFile() code. With Regards, Amit Kapila. 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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
On Wed, Aug 21, 2013 at 7:29 AM, Bruce Momjian br...@momjian.us wrote: On Tue, Aug 20, 2013 at 08:38:52AM +0530, Amit Kapila wrote: On Tue, Aug 20, 2013 at 8:27 AM, Stephen Frost sfr...@snowman.net wrote: * Amit Kapila (amit.kapil...@gmail.com) wrote: If both are okay, then I would like to go with second option (include file mechanism). I think by default, we should allow auto file to be included and if user faces any problem or otherwise, then he can decide to disable it. If it's enabled by default, then we need to ship an 'auto' file which is empty by default... initdb will create the empty auto file. If we don't enable by default, then if user uses ALTER SYSTEM and do sighup/restart, changed config parameter values will not come into affect until he manually enables it by removing '#' from '#include'. Just a heads-up, but a lot of C language folks are going to look at: #include abc and think that is enabled. True, but generally for conf and script file, symbol '#' is treated as commented portion of content. With Regards, Amit Kapila. 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
