Re: [HACKERS] [bug fix] Memory leak in dblink
From: Tom Lane t...@sss.pgh.pa.us In the first query, the MemoryContextReset is nearly free since there's nothing to free (and we've special-cased that path in aset.c). It's certainly a measurement artifact that it measures out faster, which says to me that these numbers can only be trusted within a couple percent; but at least we're not taking much hit in cases where the patch isn't actually conferring any benefit. For the second query, losing 1% to avoid memory bloat seems well worthwhile. Barring objections I'll apply and back-patch this. So this patch would solve memory leak issues if other modules had similar bugs, in addition to the original dblink problem, wouldn't this? Definitely +1. The original OP wants to use 9.2. I'll report to him when you've committed this nce patch. Thanks, Tom san. Regards MauMau -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
Hello,
Before continueing discussion, I tried this patch on the current
head.
This patch applies cleanly except one hunk because of a
modification just AFTER it, which did no harm. Finally all
regression tests suceeded.
Attached is the rebased patch of v11 up to the current master.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index 5a4d5aa..5a9aec0 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -115,6 +115,11 @@ static void fileGetForeignRelSize(PlannerInfo *root,
static void fileGetForeignPaths(PlannerInfo *root,
RelOptInfo *baserel,
Oid foreigntableid);
+static ForeignPath *fileReparameterizeForeignPath(PlannerInfo *root,
+ RelOptInfo *baserel,
+ Oid foreigntableid,
+ ForeignPath *path,
+ Relids required_outer);
static ForeignScan *fileGetForeignPlan(PlannerInfo *root,
RelOptInfo *baserel,
Oid foreigntableid,
@@ -143,7 +148,7 @@ static bool check_selective_binary_conversion(RelOptInfo *baserel,
static void estimate_size(PlannerInfo *root, RelOptInfo *baserel,
FileFdwPlanState *fdw_private);
static void estimate_costs(PlannerInfo *root, RelOptInfo *baserel,
- FileFdwPlanState *fdw_private,
+ FileFdwPlanState *fdw_private, List *join_conds,
Cost *startup_cost, Cost *total_cost);
static int file_acquire_sample_rows(Relation onerel, int elevel,
HeapTuple *rows, int targrows,
@@ -161,6 +166,7 @@ file_fdw_handler(PG_FUNCTION_ARGS)
fdwroutine-GetForeignRelSize = fileGetForeignRelSize;
fdwroutine-GetForeignPaths = fileGetForeignPaths;
+ fdwroutine-ReparameterizeForeignPath = fileReparameterizeForeignPath;
fdwroutine-GetForeignPlan = fileGetForeignPlan;
fdwroutine-ExplainForeignScan = fileExplainForeignScan;
fdwroutine-BeginForeignScan = fileBeginForeignScan;
@@ -509,7 +515,8 @@ fileGetForeignPaths(PlannerInfo *root,
(Node *) columns));
/* Estimate costs */
- estimate_costs(root, baserel, fdw_private,
+ estimate_costs(root, baserel,
+ fdw_private, NIL,
startup_cost, total_cost);
/*
@@ -534,6 +541,41 @@ fileGetForeignPaths(PlannerInfo *root,
}
/*
+ * fileReparameterizeForeignPath
+ * Attempt to modify a given path to have greater parameterization
+ */
+static ForeignPath *
+fileReparameterizeForeignPath(PlannerInfo *root,
+ RelOptInfo *baserel,
+ Oid foreigntableid,
+ ForeignPath *path,
+ Relids required_outer)
+{
+ ParamPathInfo *param_info;
+ FileFdwPlanState *fdw_private = (FileFdwPlanState *) baserel-fdw_private;
+ Cost startup_cost;
+ Cost total_cost;
+
+ /* Get the ParamPathInfo */
+ param_info = get_baserel_parampathinfo(root, baserel, required_outer);
+
+ /* Redo the cost estimates */
+ estimate_costs(root, baserel,
+ fdw_private,
+ param_info-ppi_clauses,
+ startup_cost, total_cost);
+
+ /* Make and return the new path */
+ return create_foreignscan_path(root, baserel,
+ param_info-ppi_rows,
+ startup_cost,
+ total_cost,
+ NIL, /* no pathkeys */
+ required_outer,
+ path-fdw_private);
+}
+
+/*
* fileGetForeignPlan
* Create a ForeignScan plan node for scanning the foreign table
*/
@@ -962,12 +1004,13 @@ estimate_size(PlannerInfo *root, RelOptInfo *baserel,
*/
static void
estimate_costs(PlannerInfo *root, RelOptInfo *baserel,
- FileFdwPlanState *fdw_private,
+ FileFdwPlanState *fdw_private, List *join_conds,
Cost *startup_cost, Cost *total_cost)
{
BlockNumber pages = fdw_private-pages;
double ntuples = fdw_private-ntuples;
Cost run_cost = 0;
+ QualCost join_cost;
Cost cpu_per_tuple;
/*
@@ -978,8 +1021,11 @@ estimate_costs(PlannerInfo *root, RelOptInfo *baserel,
*/
run_cost += seq_page_cost * pages;
- *startup_cost = baserel-baserestrictcost.startup;
- cpu_per_tuple = cpu_tuple_cost * 10 + baserel-baserestrictcost.per_tuple;
+ cost_qual_eval(join_cost, join_conds, root);
+ *startup_cost =
+ (baserel-baserestrictcost.startup + join_cost.startup);
+ cpu_per_tuple = cpu_tuple_cost * 10 +
+ (baserel-baserestrictcost.per_tuple + join_cost.per_tuple);
run_cost += cpu_per_tuple * ntuples;
*total_cost = *startup_cost + run_cost;
}
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 7dd43a9..3f7f7c2 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -239,6 +239,11 @@ static void postgresGetForeignRelSize(PlannerInfo *root,
static void postgresGetForeignPaths(PlannerInfo *root,
RelOptInfo *baserel,
Oid foreigntableid);
+static ForeignPath *postgresReparameterizeForeignPath(PlannerInfo *root,
+ RelOptInfo *baserel,
+ Oid foreigntableid,
+ ForeignPath *path,
+ Relids required_outer);
static ForeignScan
Re: [HACKERS] add line number as prompt option to psql
Hi Sawada Masahiko,
I liked this feature. So I have reviewed it.
Changes are straight forward and looks perfect.
No issues found with make/make install/initdb/regression.
However I would suggest removing un-necessary braces at if, as we have only
one statement into it.
if (++cur_line = INT_MAX)
{
cur_line = 1;
}
Also following looks wrong:
postgres[1]=# select
postgres[2]-# *
postgres[3]-# from
postgres[4]-# tab;
a
---
(0 rows)
postgres[1]=# select
*
from
tab
postgres[2]-# where t 10;
ERROR: column t does not exist
LINE 5: where t 10;
^
Line number in ERROR is 5 which is correct.
But line number in psql prompt is wrong.
To get first 4 lines I have simply used up arrow followed by an enter for
which I was expecting 5 in psql prompt.
But NO it was 2 which is certainly wrong.
Need to handle above carefully.
Thanks
On Thu, Jun 12, 2014 at 10:46 PM, Sawada Masahiko sawada.m...@gmail.com
wrote:
Hi all,
The attached IWP patch is one prompt option for psql, which shows
current line number.
If the user made syntax error with too long SQL then psql outputs
message as following.
ERROR: syntax error at or near a
LINE 250: hoge
^
psql teaches me where syntax error is occurred, but it is not kind
when SQL is too long.
We can use write SQL with ¥e(editor) command(e.g., emacs) , and we can
know line number.
but it would make terminal log dirty . It will make analyzing of log
difficult after error is occurred.
(I think that we usually use copy paste)
After attached this patch, we will be able to use %l option as prompting
option.
e.g.,
$ cat ~/.psqlrc
\set PROMPT2 '%/[%l]%R%# '
\set PROMPT1 '%/[%l]%R%# '
$ psql -d postgres
postgres[1]=# select
postgres[2]-# *
postgres[3]-# from
postgres[4]-# hoge;
The past discussion is following.
http://www.postgresql.org/message-id/cafj8prc1rupk6+cha1jpxph3us_zipabdovmceex4wpbp2k...@mail.gmail.com
Please give me feedback.
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
--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Phone: +91 20 30589500
Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb
This e-mail message (and any attachment) is intended for the use of the
individual or entity to whom it is addressed. This message contains
information from EnterpriseDB Corporation that may be privileged,
confidential, or exempt from disclosure under applicable law. If you are
not the intended recipient or authorized to receive this for the intended
recipient, any use, dissemination, distribution, retention, archiving, or
copying of this communication is strictly prohibited. If you have received
this e-mail in error, please notify the sender immediately by reply e-mail
and delete this message.
Re: [HACKERS] replication commands and log_statements
On Thu, Jun 12, 2014 at 10:55 PM, Robert Haas robertmh...@gmail.com wrote:
On Wed, Jun 11, 2014 at 7:42 AM, Magnus Hagander mag...@hagander.net wrote:
Replication commands like IDENTIFY_COMMAND are not logged even when
log_statements is set to all. Some users who use log_statements to
audit *all* statements might dislike this current situation. So I'm
thinking to change log_statements or add something like log_replication
so that we can log replication commands. Thought?
+1. I think adding a separate parameter is the way to go.
The other option would be to turn log_statements into a parameter that you
specify multiple ones
I kind of like this idea, but...
- so instead of all today it would be ddl,dml,all
or something like that, and then you'd also add replication as an option.
But that would cause all sorts of backwards compatibility annoyances.. And
do you really want to be able to say things like ddl,all meanin you'd get
ddl and select but not dml?
...you lost me here. I mean, I think it could be quite useful to
redefine the existing GUC as a list. We could continue to have ddl,
dml, and all as tokens that would be in the list, but you wouldn't
write ddl,dml,all because all would include everything that those
other ones would log. But then you could have combinations like
dml,replication and so on.
Yep, that seems useful, even aside from logging of replication command topic.
OK, I've just implemented the patch (attached) which does this, i.e., redefines
log_statement as a list. Thanks to the patch, log_statement can be set
to none,
ddl, mod, dml, all, and any combinations of them. The meanings of
none, ddl, mod and all are the same as they are now. New setting value
dml loggs both data modification statements like INSERT and query access
statements like SELECT and COPY TO.
What about applying this patch first?
Regards,
--
Fujii Masao
From e6a67acd0866e2b14fc716ef8a17cc7ac870e50f Mon Sep 17 00:00:00 2001
From: MasaoFujii masao.fu...@gmail.com
Date: Fri, 20 Jun 2014 20:39:08 +0900
Subject: [PATCH] Redefine log_statement as a list.
---
doc/src/sgml/config.sgml | 14 +-
src/backend/tcop/fastpath.c |2 +-
src/backend/tcop/postgres.c |3 +-
src/backend/tcop/utility.c | 60 +-
src/backend/utils/misc/guc.c | 97 ++
src/include/tcop/tcopprot.h | 15 +++---
src/include/tcop/utility.h |2 +-
7 files changed, 132 insertions(+), 61 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 8f0ca4c..e93dd37 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4490,7 +4490,7 @@ FROM pg_stat_activity;
/varlistentry
varlistentry id=guc-log-statement xreflabel=log_statement
- termvarnamelog_statement/varname (typeenum/type)
+ termvarnamelog_statement/varname (typestring/type)
indexterm
primaryvarnamelog_statement/ configuration parameter/primary
/indexterm
@@ -4498,14 +4498,16 @@ FROM pg_stat_activity;
listitem
para
Controls which SQL statements are logged. Valid values are
-literalnone/ (off), literalddl/, literalmod/, and
-literalall/ (all statements). literalddl/ logs all data definition
+literalnone/ (off), literalddl/, literalmod/, literaldml/,
+and literalall/ (all statements). literalddl/ logs all data definition
statements, such as commandCREATE/, commandALTER/, and
commandDROP/ statements. literalmod/ logs all
literalddl/ statements, plus data-modifying statements
such as commandINSERT/,
commandUPDATE/, commandDELETE/, commandTRUNCATE/,
and commandCOPY FROM/.
+literaldml/ logs all data-modifying statements, plus query access
+statements such as commandSELECT/ and commandCOPY TO/.
commandPREPARE/, commandEXECUTE/, and
commandEXPLAIN ANALYZE/ statements are also logged if their
contained command is of an appropriate type. For clients using
@@ -4515,6 +4517,12 @@ FROM pg_stat_activity;
/para
para
+This parameter can be set to a list of desired SQL statements separated by
+commas. Note that if literalnone/ is specified in the list, other settings
+are ignored and then no SQL statements are logged.
+ /para
+
+ para
The default is literalnone/. Only superusers can change this
setting.
/para
diff --git a/src/backend/tcop/fastpath.c b/src/backend/tcop/fastpath.c
index 9f50c5a..c170e54 100644
--- a/src/backend/tcop/fastpath.c
+++ b/src/backend/tcop/fastpath.c
@@ -340,7 +340,7 @@ HandleFunctionRequest(StringInfo msgBuf)
fetch_fp_info(fid, fip);
/* Log as soon as we have the function OID and name */
- if (log_statement == LOGSTMT_ALL)
+ if (log_statement LOGSTMT_OTHER)
{
ereport(LOG,
(errmsg(fastpath function call: \%s\ (OID %u),
diff --git
Re: [HACKERS] Is analyze_new_cluster.sh still useful?
Re: Tom Lane 2014-06-18 21034.1403110...@sss.pgh.pa.us Christoph Berg christoph.b...@credativ.de writes: now that we have vacuumdb --all --analyze-in-stages in 9.4, wouldn't it make sense to get rid of the analyze_new_cluster.sh file which pg_upgrade writes? The net content is a single line which could as well be printed by pg_upgrade itself. Instead of an lengthy explanation how to invoke that manually, there should be a short note and a pointer to some manual section. I think the chances of people reading that would even be increased. Similary, I don't really see the usefulness of delete_old_cluster.sh as a file, when rm -rf could just be presented on the console for the admin to execute by cut-and-paste. There are contexts where pg_upgrade is executed by some wrapper script and the user doesn't normally see its output directly. This is the case in the Red Hat packaging (unless Honza changed it since I left ;-)) and I think Debian might be similar. pg_upgradecluster shows the full pg_upgrade output in Debian. (But pg_createcluster hides the initdb output, so it the other way round here... It'd be nice if initdb would just output the interesting parts and omit most of the chatter.) I generally don't like the amount of cruft pg_upgrade leaves lying around, so I'd be glad to see these script files go away if possible; but we need to think about how this will play when there's a wrapper script between pg_upgrade and the human user. In the Red Hat wrapper script, the pg_upgrade output is dumped into a log file, which the user can look at if he wants, but I'd bet the average user doesn't read it --- that was certainly the expectation. Of course, said user probably never notices the separate shell scripts either, so maybe it's a wash. Another angle is that some folks might have tried to automate things even more, with a wrapper script that starts up the new postmaster and runs analyze_new_cluster.sh all by itself. I guess they could make the wrapper do vacuumdb --all --analyze-in-stages directly, though, so maybe that's not a fatal objection either. Yeah that was my point - that's a single static command, that could be executed by the wrapper, and it would be much less opaque. (Same for the delete script - before looking into the file you'd think it would do all sorts of cleanup, but then issues a simple rm -rf.) Mit freundlichen Grüßen, Christoph Berg -- Senior Berater, Tel.: +49 (0)21 61 / 46 43-187 credativ GmbH, HRB Mönchengladbach 12080, USt-ID-Nummer: DE204566209 Hohenzollernstr. 133, 41061 Mönchengladbach Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer pgp fingerprint: 5C48 FE61 57F4 9179 5970 87C6 4C5A 6BAB 12D2 A7AE -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] How to change the pgsql source code and build it??
On Wed, Jun 18, 2014 at 12:50 PM, Shreesha shreesha1...@gmail.com wrote: Well, the initdb issue looks to be resolved. Actually, after making the changes as suggested by Kyotaro Horiguchi, I copied only initdb binary to my platform and didn't copy all of them. Hence, the dependencies were still not resolved and was getting the error. However, now the database server is started and is up and running. But, When I try to connect the client to the server, I am getting the following error: /switch/pgsql/bin # ./psql FATAL: database root does not exist psql: FATAL: database root does not exist This is easy to solve. Your database is running. ./psql postgres you can set PGDATABASE env variable to postgres in your profile or create a database 'root' if you want to log in without arguments. I usually use the default postgres database as a scratch database or administrative database to run command like creating databases. 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] Audit of logout
On Fri, Jun 13, 2014 at 11:29 PM, Tom Lane t...@sss.pgh.pa.us wrote: Fujii Masao masao.fu...@gmail.com writes: On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao masao.fu...@gmail.com wrote: Some users enable log_disconnections in postgresql.conf to audit all logouts. But since log_disconnections is defined with PGC_BACKEND, it can be changed at connection start. This means that any client (even nonsuperuser) can freely disable log_disconnections not to log his or her logout even when the system admin enables it in postgresql.conf. Isn't this problematic for audit? That's harmful for audit purpose. I think that we should make log_disconnections PGC_SUSET rather than PGC_BACKEND in order to forbid non-superusers from changing its setting. Attached patch does this. TBH, this complaint seems entirely artificial. If somebody needs to audit disconnections, and they see a connection record with no disconnection record but the session's no longer there, they certainly know that a disconnection occurred. And if there wasn't a server crash to explain it, they go slap the wrist of whoever violated corporate policy by turning off log_disconnections. Why do we need system-level enforcement of this? Moreover, your proposed fix breaks the entire point of the PGC_BACKEND setting, which was to try to prevent disconnections from being logged or not logged when the corresponding connection was not logged or logged respectively. If an auditor wants the system to enforce that there are matched pairs of those records, he's not exactly going to be satisfied by being told that only superusers can cause them to not match. I wonder whether we should just get rid of log_disconnections as a separate variable, instead logging disconnections when log_connections is set. Another answer is to make both variables PGC_SIGHUP, on the grounds that it doesn't make much sense for them not to be applied system-wide; except that I think there was some idea that logging might be enabled per-user or per-database using ALTER ROLE/DATABASE. But, IIUC, since PGC_BACKEND parameters cannot be set per-role and per-database, such idea seems impractical. No? ISTM that there is no big user-visible problematic change of the behavior even if we redefine log_connections and log_disconnections as PGC_SIGHUP. Regards, -- Fujii Masao -- 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] replication commands and log_statements
Fujii Masao masao.fu...@gmail.com writes: OK, I've just implemented the patch (attached) which does this, i.e., redefines log_statement as a list. Thanks to the patch, log_statement can be set to none, ddl, mod, dml, all, and any combinations of them. The meanings of none, ddl, mod and all are the same as they are now. New setting value dml loggs both data modification statements like INSERT and query access statements like SELECT and COPY TO. I still don't like this one bit. It's turning log_statement from a reasonably clean design into a complete mess, which will be made even worse after you add replication control to it. 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] [WIP] showing index maintenance on EXPLAIN
On Thu, May 8, 2014 at 10:00 AM, Jaime Casanova ja...@2ndquadrant.com wrote: Hi, This patch implements $subject only when ANALYZE and VERBOSE are on. I made it that way because for years nobody seemed interested in this info (at least no one did it) so i decided that maybe is to much information for most people (actually btree indexes are normally very fast). But because we have GiST and GIN this became an interested piece of data (there are other cases even when using btree). Current patch doesn't have docs yet i will add them soon. This patch provides the Insertion time of an index operation. This information is useful to the administrator for reorganizing the indexes based on the insertion time. Quick review: Applies to Head. Regress test is passed. Coding is fine. Minor comments: There is no need of printing the index insertion time as zero in case of hot update operations. Please correct the same. Add the documentation changes. Regards, Hari Babu Fujitsu Australia -- 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] replication commands and log_statements
On Fri, Jun 20, 2014 at 10:48 PM, Tom Lane t...@sss.pgh.pa.us wrote: Fujii Masao masao.fu...@gmail.com writes: OK, I've just implemented the patch (attached) which does this, i.e., redefines log_statement as a list. Thanks to the patch, log_statement can be set to none, ddl, mod, dml, all, and any combinations of them. The meanings of none, ddl, mod and all are the same as they are now. New setting value dml loggs both data modification statements like INSERT and query access statements like SELECT and COPY TO. I still don't like this one bit. It's turning log_statement from a reasonably clean design into a complete mess, which will be made even worse after you add replication control to it. Yeah, I'd vote as well to let log_statement as it is (without mentioning the annoyance it would cause to users), and to have replication statement logging managed with a separate GUC for clarity. -- Michael -- 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] Is analyze_new_cluster.sh still useful?
On Wed, Jun 18, 2014 at 8:41 AM, Christoph Berg christoph.b...@credativ.de wrote: Hi, now that we have vacuumdb --all --analyze-in-stages in 9.4, wouldn't it make sense to get rid of the analyze_new_cluster.sh file which pg_upgrade writes? The net content is a single line which could as well be printed by pg_upgrade itself. Instead of an lengthy explanation how to invoke that manually, there should be a short note and a pointer to some manual section. I think the chances of people reading that would even be increased. That one line was longer in the past, it could become longer again in the future. I don't think we should toggle the presentation back and forth from version to version depending how long it happens to be. Similary, I don't really see the usefulness of delete_old_cluster.sh as a file, when rm -rf could just be presented on the console for the admin to execute by cut-and-paste. I certainly would not want to run rm -rf commands copied off the console window. A slip of the mouse (or the paste buffer) and suddenly you are removing entirely the wrong level of the directory tree. But I wouldn't mind an option to suppress the creation of those files. Cheers, Jeff -- 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] Is analyze_new_cluster.sh still useful?
Re: Jeff Janes 2014-06-20 CAMkU=1z3Edq+CNRo4F=jBEzXNMidSskdm=cpcaznogdy2si...@mail.gmail.com On Wed, Jun 18, 2014 at 8:41 AM, Christoph Berg christoph.b...@credativ.de wrote: Hi, now that we have vacuumdb --all --analyze-in-stages in 9.4, wouldn't it make sense to get rid of the analyze_new_cluster.sh file which pg_upgrade writes? The net content is a single line which could as well be printed by pg_upgrade itself. Instead of an lengthy explanation how to invoke that manually, there should be a short note and a pointer to some manual section. I think the chances of people reading that would even be increased. That one line was longer in the past, it could become longer again in the future. I don't think we should toggle the presentation back and forth from version to version depending how long it happens to be. I doubt that would happen. If there's more than analyze to do after the upgrade, the file would get a new name, so the API would change anyway. Similary, I don't really see the usefulness of delete_old_cluster.sh as a file, when rm -rf could just be presented on the console for the admin to execute by cut-and-paste. I certainly would not want to run rm -rf commands copied off the console window. A slip of the mouse (or the paste buffer) and suddenly you are removing entirely the wrong level of the directory tree. Well I don't like shell scripts containing rm -rf commands sitting in the filesystem either. But I wouldn't mind an option to suppress the creation of those files. Another nitpick here: What pg_upgrade outputs doesn't even work on most systems, you need to ./analyze_new_cluster.sh or sh analyze_new_cluster.sh. Mit freundlichen Grüßen, Christoph Berg -- Senior Berater, Tel.: +49 (0)21 61 / 46 43-187 credativ GmbH, HRB Mönchengladbach 12080, USt-ID-Nummer: DE204566209 Hohenzollernstr. 133, 41061 Mönchengladbach Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer pgp fingerprint: 5C48 FE61 57F4 9179 5970 87C6 4C5A 6BAB 12D2 A7AE -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] modify custom variables
Hi, When creating postgis extension, a call to DefineCustomStringVariable is made to define postgis.backend. An assign hook allows to change the library used by spatial functions whith a: SET postgis.backend = 'sfcgal'; -- or 'geos' Everything work fine execept when upgrading postgis. If I understand the problem correctly, on upgrate (ALTER EXTENSION), the old postgis library is already loaded, the old spatial functions are replaced by new ones pointing to the new postgis library, a call to DefineCustomStringVariable is made and: ERROR: attempt to redefine parameter postgis.backend I need to redefine the assign hook such that is calls the new library backend and not the old one. Is there a way to remove/modify custom variables ? I looked in guc.h but couldn't find anything. Thanks, V. PS: I'm working on http://trac.osgeo.org/postgis/ticket/2382.
Re: [HACKERS] modify custom variables
Vincent Mora vincent.m...@oslandia.com writes: If I understand the problem correctly, on upgrate (ALTER EXTENSION), the old postgis library is already loaded, the old spatial functions are replaced by new ones pointing to the new postgis library, a call to DefineCustomStringVariable is made and: ERROR: attempt to redefine parameter postgis.backend I need to redefine the assign hook such that is calls the new library backend and not the old one. I think what you need to do is arrange the upgrade process so that the old shared library doesn't get loaded in the same session as the new one. Otherwise, you're likely to have lots more problems than just this. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Shouldn't pg_(sh)seclabel.provider be marked NOT NULL?
Some of my Salesforce colleagues are looking into making every system catalog be declared with a true primary key. They came across the fact that pg_seclabel and pg_shseclabel are declared with unique indexes that include the provider column, but that column does not get marked as NOT NULL during initdb. Shouldn't it be? For that matter, it doesn't look to me like the code intends to ever store a null value into the label column either --- should that also be marked NOT NULL? I think we've generally been lazy about marking variable-width catalog columns as NOT NULL (note bootstrap mode will mark fixed-width columns as NOT NULL automatically). I'm not necessarily arguing to try to clean this up altogether, but it would be good I think if indexable columns got marked NOT NULL whenever possible. 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] JSON and Postgres Variable Queries
On 06/20/2014 11:26 AM, Joey Caughey wrote:
I’m having an issue with JSON requests in Postgres and was wondering
if anyone had an answer.
I have an orders table with a field called “json_data”.
In the json data there is a plan’s array with an id value in them.
{ plan”: { “id”: “1” } } }
I can do regular queries that will work, like so:
SELECT json_data-’plan'-’id' as plan_id FROM orders;
But if I try to query on the data that is returned it will fail:
SELECT json_data-’plan'-’id' as plan_id FROM orders WHERE plan_id = 1;
OR
SELECT json_data-’plan'-’id' as plan_id FROM orders GROUP BY plan_id;
OR
SELECT json_data-’plan'-’id' as plan_id FROM orders ORDER BY plan_id;
Is this something that has been overlooked? or is there another way to
go about this?
I’ve tried everything from the documentation here:
http://www.postgresql.org/docs/9.3/static/functions-json.html
I’ve attached a json dump of the orders table.
The double arrow operators return text, the single arrow operators
return json.
You might also find json_extract_path() useful.
BTW, this is a usage question, and as such should have gone to
pgsql-general, not pgsql-hackers.
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] [bug fix] Memory leak in dblink
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 06/19/2014 03:38 PM, MauMau wrote: From: Joe Conway m...@joeconway.com Fair enough -- this patch does it at that level in materializeQueryResult() I'm in favor of this. TBH, I did this at first, but I was afraid this would be rejected due to the reason mentioned earlier. if statement in PG_TRY block is not necessary like this, because sinfo is zero-cleared. Right -- pushed that way, back-patched through 9.2 Joe - -- Joe Conway credativ LLC: http://www.credativ.us Linux, PostgreSQL, and general Open Source Training, Service, Consulting, 24x7 Support -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.14 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJTpIv6AAoJEDfy90M199hlQtIP/ilpExgyHa86DaMCej4+oRI/ nO06AujnF+wcw/xre8xpu3sYCLebBnSbmKlqv17ry+mPfWD2KsrwMnqFgm2UoQGQ zDB/fpz4ALfQHlWrdvBqKd/J2IcdtuWaS9xi0kqShuSXWWm4XMaUIZ+gxDAA+x4U OXRumrv+kipHTONwMporZfby5DPtaRLpuV/o4ioOB4elb2VQAlajR5Vpmguhihdf A6exIN6dHIKTT2jNUHfkqnd7bZ2anVaohuociM/j+JwKaRct9K+anR3bokfLKLW9 l/OQ+BHzLBDz/Pi7GB/ImmkrIKL33phbhCWWPN1nkD6OYfYohkYvl+aixZ+kvUav wEBXTUkkJkuIL4at/5v4jbDDlPXAaYdBmGLQ/riwAOzISq2weAqcAQqhidJUbY1a JRSgojNHbo4v8IfjEj3BMRmPlKhY6Z/eMELr2Yi+K+54Hk4Fy+UDaatyDpEo2iwm cx2auCWBaT5SzI+KbebO0WZNhSrt7m1OEWN2tmPyTsnPsGg7I1oyQ/UtybaNjGtD G16HIfe12wca9Sm8zu+ypnxl3gUeku5KB0ZtigNwMxZSJXm/qa4kZqGn1RoItDnh kcfk8LTGNR1xMrPCiD+rUHyY573g8WK1XpdTWDBER29vgETdaswqXDeWsoPWmX/3 KICzxLurjqvyiJW9L4O+ =6It5 -END PGP SIGNATURE- -- 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] Shouldn't pg_(sh)seclabel.provider be marked NOT NULL?
On Jun 20, 2014, at 10:01 AM, Tom Lane t...@sss.pgh.pa.us wrote: Some of my Salesforce colleagues are looking into making every system catalog be declared with a true primary key. They came across the fact that pg_seclabel and pg_shseclabel are declared with unique indexes that include the provider column, but that column does not get marked as NOT NULL during initdb. Shouldn't it be? At some point, I inferred a rule that catalog columns were intended to be either both fixed-width and not nullable; or variable-width and nullable. I believe the current situation is the result of that inference... but I see no particular reason not to change it. ...Robert -- 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] Shouldn't pg_(sh)seclabel.provider be marked NOT NULL?
Robert Haas robertmh...@gmail.com writes: On Jun 20, 2014, at 10:01 AM, Tom Lane t...@sss.pgh.pa.us wrote: Some of my Salesforce colleagues are looking into making every system catalog be declared with a true primary key. They came across the fact that pg_seclabel and pg_shseclabel are declared with unique indexes that include the provider column, but that column does not get marked as NOT NULL during initdb. Shouldn't it be? At some point, I inferred a rule that catalog columns were intended to be either both fixed-width and not nullable; or variable-width and nullable. I believe the current situation is the result of that inference... but I see no particular reason not to change it. The actual rule that's embodied in the bootstrap code is to mark everything that could potentially be referenced via a C struct field as not nullable: which is to say, fixed-width fields up till we get to the first variable-width one. It's fairly likely that this is *not* marking all the columns that the code expects to be non-null in practice. The idea I'm toying with right now is to additionally mark as not nullable any column referenced in a DECLARE_UNIQUE_INDEX command in catalog/indexing.h. But I've not looked through that set carefully; it's conceivable that we actually have some indexed catalog columns that are allowed to be null. A possibly better solution is to invent a new macro that has the same semantics as DECLARE_UNIQUE_INDEX, plus forcing the columns to be marked NOT NULL. A bigger-picture question is whether there are yet more columns that could be marked not null, and how much we care about making them so. 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] Shouldn't pg_(sh)seclabel.provider be marked NOT NULL?
Tom Lane wrote: The idea I'm toying with right now is to additionally mark as not nullable any column referenced in a DECLARE_UNIQUE_INDEX command in catalog/indexing.h. But I've not looked through that set carefully; it's conceivable that we actually have some indexed catalog columns that are allowed to be null. A possibly better solution is to invent a new macro that has the same semantics as DECLARE_UNIQUE_INDEX, plus forcing the columns to be marked NOT NULL. I think most, if not all, the unique indexes declared are part of a syscache. I don't think we allow those to be null, so in effect those columns are already not nullable. Non-unique indexes in indexing.h already bear a standard comment that they are not used for syscache. The only exception was added recently in f01d1ae3a104019: DECLARE_INDEX(pg_class_tblspc_relfilenode_index, 3455, on pg_class using btree(reltablespace oid_ops, relfilenode oid_ops)); -- Á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] Shouldn't pg_(sh)seclabel.provider be marked NOT NULL?
On 2014-06-20 16:50:15 -0400, Alvaro Herrera wrote: Tom Lane wrote: The idea I'm toying with right now is to additionally mark as not nullable any column referenced in a DECLARE_UNIQUE_INDEX command in catalog/indexing.h. But I've not looked through that set carefully; it's conceivable that we actually have some indexed catalog columns that are allowed to be null. A possibly better solution is to invent a new macro that has the same semantics as DECLARE_UNIQUE_INDEX, plus forcing the columns to be marked NOT NULL. I think most, if not all, the unique indexes declared are part of a syscache. I don't think we allow those to be null, so in effect those columns are already not nullable. Non-unique indexes in indexing.h already bear a standard comment that they are not used for syscache. The only exception was added recently in f01d1ae3a104019: DECLARE_INDEX(pg_class_tblspc_relfilenode_index, 3455, on pg_class using btree(reltablespace oid_ops, relfilenode oid_ops)); There's no NULLs in here. It can have duplicates, but in that it's far from alone. 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] Shouldn't pg_(sh)seclabel.provider be marked NOT NULL?
Andres Freund and...@2ndquadrant.com writes: On 2014-06-20 16:50:15 -0400, Alvaro Herrera wrote: Non-unique indexes in indexing.h already bear a standard comment that they are not used for syscache. The only exception was added recently in f01d1ae3a104019: DECLARE_INDEX(pg_class_tblspc_relfilenode_index, 3455, on pg_class using btree(reltablespace oid_ops, relfilenode oid_ops)); There's no NULLs in here. It can have duplicates, but in that it's far from alone. I think Alvaro was complaining that it's alone in lacking this comment: /* This following index is not used for a cache and is not unique */ But TBH, I don't think those comments are worth much. I'd rather get rid of them all and instead add an Assert to the cache code enforcing that any index underlying a catcache is unique. It looks like the easiest place to do that is InitCatCachePhase2 --- that's the only place in catcache.c that actually opens the underlying index directly. I'd like to also have an Assert in there that the index columns are marked NOT NULL, but not sure if they actually all are marked that way today. 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] Shouldn't pg_(sh)seclabel.provider be marked NOT NULL?
On 2014-06-20 17:29:33 -0400, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2014-06-20 16:50:15 -0400, Alvaro Herrera wrote: Non-unique indexes in indexing.h already bear a standard comment that they are not used for syscache. The only exception was added recently in f01d1ae3a104019: DECLARE_INDEX(pg_class_tblspc_relfilenode_index, 3455, on pg_class using btree(reltablespace oid_ops, relfilenode oid_ops)); There's no NULLs in here. It can have duplicates, but in that it's far from alone. I think Alvaro was complaining that it's alone in lacking this comment: /* This following index is not used for a cache and is not unique */ But TBH, I don't think those comments are worth much. I'd rather get rid of them all and instead add an Assert to the cache code enforcing that any index underlying a catcache is unique. It looks like the easiest place to do that is InitCatCachePhase2 --- that's the only place in catcache.c that actually opens the underlying index directly. I'd like to also have an Assert in there that the index columns are marked NOT NULL, but not sure if they actually all are marked that way today. Sounds sensible. If they aren't marking them as such hopefully isn't problematic... 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] Re: [BUGS] BUG #8673: Could not open file pg_multixact/members/xxxx on slave during hot_standby
Jeff Janes wrote: 12538 2014-06-17 12:10:02.925 PDT:LOG: JJ deleting 0xb66b20 5183 12498 UPDATE 2014-06-17 12:10:03.188 PDT:DETAIL: Could not open file pg_multixact/members/5183: No such file or directory. 12561 UPDATE 2014-06-17 12:10:04.473 PDT:DETAIL: Could not open file pg_multixact/members/5183: No such file or directory. 12572 UPDATE 2014-06-17 12:10:04.475 PDT:DETAIL: Could not open file pg_multixact/members/5183: No such file or directory. This problem was initially fairly easy to reproduce, but since I started adding instrumentation specifically to catch it, it has become devilishly hard to reproduce. I think I see the problem here now, after letting this test rig run for a while. First, the fact that there are holes in members/ files because of the random order in deletion, in itself, seems harmless, because the files remaining in between will be deleted by a future vacuum. Now, the real problem is that we delete files during vacuum, but the state that marks those file as safely deletable is written as part of a checkpoint record, not by vacuum itself (vacuum writes its state in pg_database, but a checkpoint derives its info from a shared memory variable.) Taken together, this means that if there's a crash between the vacuum that does a deletion and the next checkpoint, we might attempt to read an offset file that is not supposed to be part of the live range -- but we forgot that because we didn't reach the point where we save the shmem state to disk. It seems to me that we need to keep the offsets files around until a checkpoint has written the oldest number to WAL. In other words we need additional state in shared memory: (a) what we currently store which is the oldest number as computed by vacuum (not safe to delete, but it's the number that the next checkpoint must write), and (b) the oldest number that the last checkpoint wrote (the safe deletion point). Another thing I noticed is that more than one vacuum process can try to run deletion simultaneously, at least if they crash frequently while they were doing deletion. I don't see that this is troublesome, even though they might attempt to delete the same files. Finally, I noticed that we first read the oldest offset file, then determine the member files to delete; then delete offset files, then delete member files. Which means that we would delete offset files that correspond to member files that we keep (assuming there is a crash in the middle of deletion). It seems to me we should first delete members, then delete offsets, if we wanted to be very safe about it. I don't think this really would matters much, if we were to do things safely as described above. -- Á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] Shouldn't pg_(sh)seclabel.provider be marked NOT NULL?
Andres Freund wrote: On 2014-06-20 16:50:15 -0400, Alvaro Herrera wrote: I think most, if not all, the unique indexes declared are part of a syscache. I don't think we allow those to be null, so in effect those columns are already not nullable. Non-unique indexes in indexing.h already bear a standard comment that they are not used for syscache. The only exception was added recently in f01d1ae3a104019: DECLARE_INDEX(pg_class_tblspc_relfilenode_index, 3455, on pg_class using btree(reltablespace oid_ops, relfilenode oid_ops)); There's no NULLs in here. It can have duplicates, but in that it's far from alone. I'm only saying it's missing the /* this index is not unique */ comment that all other DECLARE_INDEX() lines have. Sorry I wasn't clear. -- Á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] Shouldn't pg_(sh)seclabel.provider be marked NOT NULL?
Andres Freund and...@2ndquadrant.com writes: On 2014-06-20 17:29:33 -0400, Tom Lane wrote: I think Alvaro was complaining that it's alone in lacking this comment: /* This following index is not used for a cache and is not unique */ But TBH, I don't think those comments are worth much. I'd rather get rid of them all and instead add an Assert to the cache code enforcing that any index underlying a catcache is unique. It looks like the easiest place to do that is InitCatCachePhase2 --- that's the only place in catcache.c that actually opens the underlying index directly. I'd like to also have an Assert in there that the index columns are marked NOT NULL, but not sure if they actually all are marked that way today. Sounds sensible. If they aren't marking them as such hopefully isn't problematic... Experimental result from adding an Assert in CatalogCacheInitializeCache is that it doesn't blow up :-). So we do have them all marked correctly. 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] Extended Prefetching using Asynchronous IO - proposal and patch
Thanks Fujii , that is a bug -- an #ifdef USE_PREFETCH is missing in heapam.c (maybe several) I will fix it in the next patch version. I also appreciate it is not easy to review the patch. There are really 4 (or maybe 5) parts : . async io (librt functions) . buffer management (allocating, locking and pinning etc) . scanners making prefetch calls . statistics and the autoconf input program I will see what I can do. Maybe putting an indicator against each modified file? I am currently working on two things : . alternative way for non-originator of an aio_read to wait on completion (LWlock instead of polling the aiocb) This was talked about in several earlier posts and Claudio is also working on something there . package up my benchmark Cheers John Date: Fri, 20 Jun 2014 04:21:19 +0900 Subject: Re: [HACKERS] Extended Prefetching using Asynchronous IO - proposal and patch From: masao.fu...@gmail.com To: johnlu...@hotmail.com CC: pgsql-hackers@postgresql.org; klaussfre...@gmail.com On Mon, Jun 9, 2014 at 11:12 AM, johnlumby johnlu...@hotmail.com wrote: updated version of patch compatible with git head of 140608, (adjusted proc oid and a couple of minor fixes) Compilation of patched version on MacOS failed. The error messages were gcc -O0 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -g -I../../../../src/include -c -o heapam.o heapam.c heapam.c: In function 'heap_unread_add': heapam.c:362: error: 'struct HeapScanDescData' has no member named 'rs_Unread_Pfetched_next' heapam.c:363: error: 'struct HeapScanDescData' has no member named 'rs_Unread_Pfetched_count' heapam.c:369: error: 'struct HeapScanDescData' has no member named 'rs_Unread_Pfetched_count' heapam.c:375: error: 'struct HeapScanDescData' has no member named 'rs_Unread_Pfetched_base' heapam.c:381: error: 'struct HeapScanDescData' has no member named 'rs_Unread_Pfetched_base' heapam.c:387: error: 'struct HeapScanDescData' has no member named 'rs_Unread_Pfetched_next' heapam.c:405: error: 'struct HeapScanDescData' has no member named 'rs_Unread_Pfetched_count' heapam.c: In function 'heap_unread_subtract': heapam.c:419: error: 'struct HeapScanDescData' has no member named 'rs_Unread_Pfetched_next' heapam.c:425: error: 'struct HeapScanDescData' has no member named 'rs_Unread_Pfetched_count' heapam.c:434: error: 'struct HeapScanDescData' has no member named 'rs_Unread_Pfetched_base' heapam.c:442: error: 'struct HeapScanDescData' has no member named 'rs_Unread_Pfetched_base' heapam.c:452: error: 'struct HeapScanDescData' has no member named 'rs_Unread_Pfetched_next' heapam.c:453: error: 'struct HeapScanDescData' has no member named 'rs_Unread_Pfetched_next' heapam.c:454: error: 'struct HeapScanDescData' has no member named 'rs_Unread_Pfetched_next' heapam.c:456: error: 'struct HeapScanDescData' has no member named 'rs_Unread_Pfetched_count' heapam.c: In function 'heapgettup': heapam.c:944: error: 'struct HeapScanDescData' has no member named 'rs_pfchblock' heapam.c:716: warning: unused variable 'ix' heapam.c: In function 'heapgettup_pagemode': heapam.c:1243: error: 'struct HeapScanDescData' has no member named 'rs_pfchblock' heapam.c:1029: warning: unused variable 'ix' heapam.c: In function 'heap_endscan': heapam.c:1808: error: 'struct HeapScanDescData' has no member named 'rs_Unread_Pfetched_base' heapam.c:1809: error: 'struct HeapScanDescData' has no member named 'rs_Unread_Pfetched_base' make[4]: *** [heapam.o] Error 1 make[3]: *** [heap-recursive] Error 2 make[2]: *** [access-recursive] Error 2 make[1]: *** [install-backend-recurse] Error 2 make: *** [install-src-recurse] Error 2 Huge patch is basically not easy to review. What about simplifying the patch by excluding non-core parts like the change of pg_stat_statements, so that reviewers can easily read the patch? We can add such non-core parts later. Regards, -- Fujii Masao -- 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] Re: [BUGS] BUG #8673: Could not open file pg_multixact/members/xxxx on slave during hot_standby
On 2014-06-20 17:38:16 -0400, Alvaro Herrera wrote: Jeff Janes wrote: 12538 2014-06-17 12:10:02.925 PDT:LOG: JJ deleting 0xb66b20 5183 12498 UPDATE 2014-06-17 12:10:03.188 PDT:DETAIL: Could not open file pg_multixact/members/5183: No such file or directory. 12561 UPDATE 2014-06-17 12:10:04.473 PDT:DETAIL: Could not open file pg_multixact/members/5183: No such file or directory. 12572 UPDATE 2014-06-17 12:10:04.475 PDT:DETAIL: Could not open file pg_multixact/members/5183: No such file or directory. This problem was initially fairly easy to reproduce, but since I started adding instrumentation specifically to catch it, it has become devilishly hard to reproduce. I think I see the problem here now, after letting this test rig run for a while. First, the fact that there are holes in members/ files because of the random order in deletion, in itself, seems harmless, because the files remaining in between will be deleted by a future vacuum. Now, the real problem is that we delete files during vacuum, but the state that marks those file as safely deletable is written as part of a checkpoint record, not by vacuum itself (vacuum writes its state in pg_database, but a checkpoint derives its info from a shared memory variable.) Taken together, this means that if there's a crash between the vacuum that does a deletion and the next checkpoint, we might attempt to read an offset file that is not supposed to be part of the live range -- but we forgot that because we didn't reach the point where we save the shmem state to disk. It seems to me that we need to keep the offsets files around until a checkpoint has written the oldest number to WAL. In other words we need additional state in shared memory: (a) what we currently store which is the oldest number as computed by vacuum (not safe to delete, but it's the number that the next checkpoint must write), and (b) the oldest number that the last checkpoint wrote (the safe deletion point). Why not just WAL log truncations? If we'd emit the WAL record after determining the offsets page we should be safe I think? That seems like easier and more robust fix? And it's what e.g. the clog does. Another thing I noticed is that more than one vacuum process can try to run deletion simultaneously, at least if they crash frequently while they were doing deletion. I don't see that this is troublesome, even though they might attempt to delete the same files. That actually seems to be bad to me. It might fail to fail, but still. Finally, I noticed that we first read the oldest offset file, then determine the member files to delete; then delete offset files, then delete member files. Which means that we would delete offset files that correspond to member files that we keep (assuming there is a crash in the middle of deletion). It seems to me we should first delete members, then delete offsets, if we wanted to be very safe about it. I don't think this really would matters much, if we were to do things safely as described above. Part of that seems to be solveable by WAL logging truncations. But I also think that we need a more robust tracking of the oldest member offset - we still aren't safe against member wraparounds. And I don't see how we can be without explicitly tracking the oldest member instead of the ugly 'search for oldest offset segment and map that to members' thing we're doing 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] Window function optimisation, allow pushdowns of items matching PARTITION BY clauses
I've had a chance to look at this and here is my review.
On 04/14/2014 01:19 PM, David Rowley wrote:
I've included the updated patch with some regression tests.
The first thing I noticed is there is no documentation, but I don't
think we document such things outside of the release notes, so that's
probably normal.
The final test I added tests that an unused window which would, if used,
cause the pushdown not to take place still stops the pushdownf from
happening... I know you both talked about this case in the other thread,
but I've done nothing for it as Tom mentioned that this should likely be
fixed elsewhere, if it's even worth worrying about at all. I'm not quite
sure if I needed to bother including this test for it, but I did anyway,
it can be removed if it is deemed unneeded.
I don't think this test has any business being in this patch. Removal
of unused things should be a separate patch and once that's done your
patch should work as expected. This regression test you have here
almost demands that that other removal patch shouldn't happen.
Per Thomas' comments, I added a couple of tests that ensure that the
order of the columns in the partition by clause don't change the
behaviour. Looking at the implementation of the actual code makes this
test seems a bit unneeded as really but I thought that the test should
not assume anything about the implementation so from that point of view
the extra test seems like a good idea.
Every possible permutation of everything is overkill, but I think having
a test that makes sure the pushdown happens when the partition in
question isn't first in the list is a good thing.
For now I can't think of much else to do for the patch, but I'd really
appreciate it Thomas if you could run it through the paces if you can
find any time for it, or maybe just give my regression tests a once over
to see if you can think of any other cases that should be covered.
I'm not Thomas, but...
This patch is very simple. There's nothing wrong with that, of course,
but I wonder how hard it would be to push more stuff down. For example,
you have this case which works perfectly by not pushing down:
SELECT * FROM (
SELECT partid,
winagg() OVER (PARTITION BY partid+0)
FROM t) wa
WHERE partid = 1;
But then you have this case which doesn't push down:
SELECT * FROM (
SELECT partid,
winagg() OVER (PARTITION BY partid+0)
FROM t) wa
WHERE partid+0 = 1;
I don't know what's involved in fixing that, or if we do that sort of
thing elsewhere, but it's something to consider.
Multi-column pushdowns work as expected.
I have an issue with some of the code comments:
In subquery_is_pushdown_safe you reduced the number of points from
three to two. The comments used to say Check point 1 and Check point
2 but the third was kind of tested throughout the rest of the code so
didn't have a dedicated comment. Now that there are only two checks,
the Check point 1 without a 2 seems a little bit odd. The attached
patch provides a suggestion for improvement.
The same kind of weirdness is found in check_output_expressions but I
don't really have any suggestions to fix it so I just left it.
The attached patch also fixes some typos.
I'm marking this Waiting on Author pending discussion on pushing down
entire expressions, but on the whole I think this is pretty much ready.
--
Vik
*** a/src/backend/optimizer/path/allpaths.c
--- b/src/backend/optimizer/path/allpaths.c
***
*** 1692,1698 subquery_is_pushdown_safe(Query *subquery, Query *topquery,
{
SetOperationStmt *topop;
! /* Check point 1 */
if (subquery-limitOffset != NULL || subquery-limitCount != NULL)
return false;
--- 1692,1698
{
SetOperationStmt *topop;
! /* Pushdown is unsafe if we have a LIMIT/OFFSET clause */
if (subquery-limitOffset != NULL || subquery-limitCount != NULL)
return false;
***
*** 1794,1802 recurse_pushdown_safe(Node *setOp, Query *topquery,
*
* 4. If the subquery has windowing functions we are able to push down any
* quals that are referenced in all of the subquery's window PARTITION BY
! * clauses. This is permitted as window partitions are completed isolated
* from each other and removing records from unneeded partitions early has
! * no affect on the query results.
*/
static void
check_output_expressions(Query *subquery, bool *unsafeColumns)
--- 1794,1802
*
* 4. If the subquery has windowing functions we are able to push down any
* quals that are referenced in all of the subquery's window PARTITION BY
! * clauses. This is permitted as window partitions are completely isolated
* from each other and removing records from unneeded partitions early has
! * no effect on the query results.
*/
static void
check_output_expressions(Query *subquery, bool *unsafeColumns)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes
Re: [HACKERS] API change advice: Passing plan invalidation info from the rewriter into the planner?
Robert, * Robert Haas (robertmh...@gmail.com) wrote: On Wed, Jun 18, 2014 at 10:40 AM, Stephen Frost sfr...@snowman.net wrote: * Robert Haas (robertmh...@gmail.com) wrote: Technically, there are 4 bits left, and that's what we need for separate privileges. I'd really hate to chew them all up.. Usually it's the patch author who WANTS to chew up all the available bit space and OTHER people who say no. :-) Ah, well, technically I'm not the patch author here, though I would like to see it happen. :) Still, have to balance these features and capabilities against the future unknown options we might want to add and it certainly doesn't seem terribly nice to chew up all that remain rather than addressing the need to support more. Still, perhaps we can put together a patch for this and then review the implementation and, if we like it and that functionality, we can make the decision about if it should be on this patch to make more bits available. Perhaps, or we might come up with some new whiz-bang permission to add next year. :/ Well, people proposed separate permissions for things like VACUUM and ANALYZE around the time TRUNCATE was added, and those were rejected on the grounds that they didn't add enough value to justify wasting bits on them. I think we see whether there's a workable system that such that marginal permissions (like TRUNCATE) that won't be checked often don't have to consume bits. That's an interesting approach but I'm not sure that we need to go a system where we segregate often-used bits from less-used ones. My thoughts on how to address this were to segregate the ACL bits by object type. That is to say, the AclMode stored for databases might only use bits 0-2 (create/connect/temporary), while tables would use bits 0-7 (insert/select/update/delete/references/trigger). This would allow us to more easily add more rights at the database and/or tablespace level too. Yeah, that's another idea. But it really deserves its own thread. I'm still not convinced we have to do this at all to meet this need, but that should be argued back and forth on that other thread. Fair enough. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] API change advice: Passing plan invalidation info from the rewriter into the planner?
Stephen Frost sfr...@snowman.net writes: * Robert Haas (robertmh...@gmail.com) wrote: [ counting bits in ACLs ] Wouldn't it be fairly painless to widen AclMode from 32 bits to 64, and thereby double the number of available bits? That code was all written before we required platforms to have an int64 primitive type, but of course now we expect that. In any case, I concur with the position that this feature patch should be separate from a patch to make additional bitspace available. 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] Audit of logout
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 06/13/2014 07:29 AM, Tom Lane wrote: Fujii Masao masao.fu...@gmail.com writes: On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao masao.fu...@gmail.com wrote: Some users enable log_disconnections in postgresql.conf to audit all logouts. But since log_disconnections is defined with PGC_BACKEND, it can be changed at connection start. This means that any client (even nonsuperuser) can freely disable log_disconnections not to log his or her logout even when the system admin enables it in postgresql.conf. Isn't this problematic for audit? That's harmful for audit purpose. I think that we should make log_disconnections PGC_SUSET rather than PGC_BACKEND in order to forbid non-superusers from changing its setting. Attached patch does this. This whole argument seems wrong unless I'm missing something: test=# set log_connections = on; ERROR: parameter log_connections cannot be set after connection start test=# set log_disconnections = off; ERROR: parameter log_disconnections cannot be set after connection start I wonder whether we should just get rid of log_disconnections as a separate variable, instead logging disconnections when log_connections is set. That might be a good idea though. Another answer is to make both variables PGC_SIGHUP, on the grounds that it doesn't make much sense for them not to be applied system-wide; except that I think there was some idea that logging might be enabled per-user or per-database using ALTER ROLE/DATABASE. I don't think this is a good idea because of the reason you mention. Joe - -- Joe Conway credativ LLC: http://www.credativ.us Linux, PostgreSQL, and general Open Source Training, Service, Consulting, 24x7 Support -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.14 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJTpQMcAAoJEDfy90M199hltHAP/2hEnKymoEq6zryaSpHZ2j0O mj/8bEzCgYR/S4KUW8uqCzYK0g3HD5ncXJZkqpnaYvySV5YnopeUjuHaXxZOmuxx GSbtmxo0wE5cYfEartVsX+ve0j7uSUwXBYZWD3em9FXNwFMnfVt3E/izwmHEnC7u pIFHz6wKn6/QKaU9u/XRln4SZOAzeh4aYaHZy+5mhmGoU8fIJtZvdjEJSuAxxgzm LMKGM/hgF23itpjjutDxQNoTUP+JGh0WzwqeW1t4+Y6T7HqXeTeT4IWsw3AH5sPg e/NM+x4oeX9In6Gn4MLwT4R5Qai/JnaKGpzUv0jXlWPPvB23ilsb87eJ0BdbKDu1 LyxH16bH23DYL9LW+GAULRoMP78PLMKh4Mx2pe9KSL9tEBENvYpf+ew3IOfRmTlD MAQRvhzspjPWp1AMQ9eNjX+63mpAeTBfHOBlVKUznhljHdDN7rcwpOzL82ecowDi nM9bC+Me1jabaxRdu2cxt+p28BB5Ez3CX2wOz2JpM0ObruneoFhYCKXM9fUaD1d2 zJXiNtD7VgsUUtz+DGrNB32PyvzguhK0MXpX6/kRl5L1Xkpa4L+AV1nXWCkJYD6D +btVgDscfnlWo1lQimq7B0KVET4zXnyI97vE7Xx0U7mvo8FZ8SQQHhbA7iy4P2SI HUlqaKVcx2PLgoRAEEfL =vQd8 -END PGP SIGNATURE- -- 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] API change advice: Passing plan invalidation info from the rewriter into the planner?
Tom, * Tom Lane (t...@sss.pgh.pa.us) wrote: Stephen Frost sfr...@snowman.net writes: * Robert Haas (robertmh...@gmail.com) wrote: [ counting bits in ACLs ] Wouldn't it be fairly painless to widen AclMode from 32 bits to 64, and thereby double the number of available bits? Thanks for commenting on this. I hadn't considered that but I don't see any particular problem with it either.. In any case, I concur with the position that this feature patch should be separate from a patch to make additional bitspace available. Certainly. Thanks for your thoughts. Stephen signature.asc Description: Digital signature
