Re: [HACKERS] Configuration include directory
Hi Greg,
On Tue, Nov 15, 2011 at 11:53:53PM -0500, Greg Smith wrote:
> Two years ago Magnus submitted a patch to parse all the configuration
> files in a directory. After some discussion I tried to summarize what I
> thought the most popular ideas were for moving that forward:
>
> http://archives.postgresql.org/pgsql-hackers/2009-10/msg01452.php
> http://archives.postgresql.org/pgsql-hackers/2009-10/msg01631.php
What a thread. I think the earlier patch was more controversial due to its
introduction of policy, a single include directory searched by default. This
latest patch is just infrastructure through which individual sites can build
all manner of configuration strategies. Many projects implement similar
directives for their configuration files, so I'm quite comfortable assuming
that some sites/packagers will like this.
> -If it's not an absolute path, considers that relative to the -D option
> (if specified) or PGDATA, the same logic used to locate the
> postgresql.conf (unless a full path to it is used)
Let's instead mimic the behavior of the "include" directive, which finds its
target relative to the file containing the directive. This also removes the
warts you mentioned in your final two paragraphs:
> No docs in here yet. There's one ugly bit of code here I was hoping
> (but failed) to avoid. Right now the server doesn't actually save the
> configuration directory anywhere. Once you leave the initial read in
> SelectConfigFiles, that information is gone, and you only have the
> configfile. I decided to make that configdir into a global value.
> Seemed easier than trying to pass it around, given how many SIGHUP paths
> could lead to this new code.
SelectConfigFiles() still does "free(configdir)". Due to that, in my testing,
SIGHUP reloads do not re-find relative includedirs.
> I can see some potential confusion here in one case. Let's say someone
> specifies a full path to their postgresql.conf file. They might assume
> that the includedir was relative to the directory that file is in.
> Let's say configfile is /etc/sysconfig/pgsql/postgresql.conf ; a user
> might think that "includedir conf.d" from there would reference
> /etc/sysconfig/pgsql/conf.d instead of the $PGDATA/conf.d you actually
> get. Wavering on how to handle that is one reason I didn't try
> documenting this yet, the decision I made here may not actually be the
> right one.
For this patch, the documentation is perhaps more important than the code.
> *** a/src/backend/utils/misc/guc-file.l
> --- b/src/backend/utils/misc/guc-file.l
> *** ParseConfigFp(FILE *fp, const char *conf
> *** 599,604
> --- 616,727
> return OK;
> }
>
> + static int
> + comparestr(const void *a, const void *b)
> + {
> + return strcmp(*(char **) a, *(char **) b);
> + }
> +
> + /*
> + * Read and parse all config files in a subdirectory in alphabetical order
> + */
> + bool
> + ParseConfigDirectory(const char *includedir,
> + const char *calling_file,
> + int depth, int elevel,
> + ConfigVariable **head_p,
> + ConfigVariable **tail_p)
> + {
> + DIR *d;
> + struct dirent *de;
> + char directory[MAXPGPATH];
> + char **filenames = NULL;
> + int num_filenames = 0;
> + int size_filenames = 0;
> + bool status;
> +
> + if (is_absolute_path(includedir))
> + sprintf(directory, "%s", includedir);
> + else
> + sprintf(directory, "%s/%s", configdir, includedir);
You need a length-limiting copy, and this won't cut it on Windows. I suggest
extracting and reusing the comparable logic in ParseConfigFile().
> + d = AllocateDir(directory);
> + if (d == NULL)
> + {
> + /*
> + * Not finding the configuration directory is not fatal,
> because we
> + * still have the main postgresql.conf file. Return true so the
> + * complete config parsing doesn't fail in this case. Also avoid
> + * logging this, since it can be a normal situtation.
> + */
> + return true;
I can't see much to recommend this; it's morally equivalent to silently
ignoring "include somefile" or "work_mem = 'foobar'".
> + }
> +
> + /*
> + * Read the directory and put the filenames in an array, so we can sort
> + * them prior to processing the contents.
> + */
> + while ((de = ReadDir(d, directory)) != NULL)
> + {
> + struct stat st;
> + char filename[MAXPGPATH];
> +
> + /*
> + * Only parse files with names ending in ".conf".
> + * This automatically excludes things like "." and "..", as well
> + * as backup files and editor debris.
> + */
> + if (strlen(de->d_name) < 6)
> + continue;
I would probab
[HACKERS] xlog location arithmetic
Hi,
A while ago when blogging about WAL [1], I noticed a function to deal with
xlog location arithmetic is wanted. I remembered Depez [2] mentioning it and
after some questions during trainings and conferences I decided to translate
my shell script function in C.
The attached patch implements the function pg_xlog_location_diff (bikeshed
colors are welcome). It calculates the difference between two given
transaction log locations. Now that we have pg_stat_replication view, it will
be easy to get the lag just passing columns as parameters. Also, the
monitoring tools could take advantage of it instead of relying on a fragile
routine to get the lag.
I noticed that pg_xlogfile_name* functions does not sanity check the xrecoff
boundaries but that is material for another patch.
[1] http://eulerto.blogspot.com/2011/11/understanding-wal-nomenclature.html
[2]
http://www.depesz.com/index.php/2011/01/24/waiting-for-9-1-pg_stat_replication/
--
Euler Taveira de Oliveira - Timbira http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index ddfb29a..cce218a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -14342,6 +14342,9 @@ SELECT set_config('log_statement_stats', 'off', false);
pg_xlogfile_name_offset
+
+pg_xlog_location_diff
+
The functions shown in text, integer
Convert transaction log location string to file name and decimal byte offset within file
+
+
+pg_xlog_location_diff(location text, location text)
+
+ bigint
+ Calculate the difference between two transaction log locations
+
@@ -14507,6 +14517,13 @@ postgres=# SELECT * FROM pg_xlogfile_name_offset(pg_stop_backup());
+ pg_xlog_location_diff calculates the difference in bytes
+ between two transaction log locations. It can be used with
+ pg_stat_replication or some functions shown in
+ to get the replication lag.
+
+
+
For details about proper usage of these functions, see
.
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index 22c6ca0..09e8369 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -26,6 +26,7 @@
#include "replication/walreceiver.h"
#include "storage/smgr.h"
#include "utils/builtins.h"
+#include "utils/int8.h"
#include "utils/guc.h"
#include "utils/timestamp.h"
@@ -465,3 +466,57 @@ pg_is_in_recovery(PG_FUNCTION_ARGS)
{
PG_RETURN_BOOL(RecoveryInProgress());
}
+
+/*
+ * Compute the difference in bytes between two WAL locations.
+ */
+Datum
+pg_xlog_location_diff(PG_FUNCTION_ARGS)
+{
+ text *location1 = PG_GETARG_TEXT_P(0);
+ text *location2 = PG_GETARG_TEXT_P(1);
+ char *str1, *str2;
+ uint32 xlogid1, xrecoff1;
+ uint32 xlogid2, xrecoff2;
+ int64 tmp;
+ int64 result;
+
+ /*
+ * Read and parse input
+ */
+ str1 = text_to_cstring(location1);
+ str2 = text_to_cstring(location2);
+
+ if (sscanf(str1, "%8X/%8X", &xlogid1, &xrecoff1) != 2)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("could not parse transaction log location \"%s\"", str1)));
+ if (sscanf(str2, "%8X/%8X", &xlogid2, &xrecoff2) != 2)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("could not parse transaction log location \"%s\"", str2)));
+
+ /*
+ * Sanity check
+ */
+ if (xrecoff1 > XLogFileSize)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("xrecoff \"%X\" is out of valid range, 0..%X", xrecoff1, XLogFileSize)));
+ if (xrecoff2 > XLogFileSize)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("xrecoff \"%X\" is out of valid range, 0..%X", xrecoff2, XLogFileSize)));
+
+ /*
+ * Use the int8 functions mainly for overflow detection
+ *
+ * result = XLogFileSize * (xlogid1 - xlogid2) + xrecoff1 - xrecoff2
+ */
+ tmp = DirectFunctionCall2(int8mi, xlogid1, xlogid2);
+ tmp = DirectFunctionCall2(int8mul, XLogFileSize, tmp);
+ tmp = DirectFunctionCall2(int8pl, tmp, xrecoff1);
+ result = DirectFunctionCall2(int8mi, tmp, xrecoff2);
+
+ PG_RETURN_INT64(result);
+}
diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h
index cb43879..3e7340b 100644
--- a/src/include/access/xlog_internal.h
+++ b/src/include/access/xlog_internal.h
@@ -279,5 +279,6 @@ extern Datum pg_is_in_recovery(PG_FUNCTION_ARGS);
extern Datum pg_xlog_replay_pause(PG_FUNCTION_ARGS);
extern Datum pg_xlog_replay_resume(PG_FUNCTION_ARGS);
extern Datum pg_is_xlog_replay_paused(PG_FUNCTION_ARGS);
+extern Datum pg_xlog_location_diff(PG_FUNCTION_ARGS);
#endif /* XLOG_INTERNAL_H */
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 28e53b7..036d6ca 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -2869,6 +2869,8 @@ DATA(insert OI
[HACKERS] pull_up_simple_subquery
While working on KaiGai's "leaky views" patch, I found myself
scrutinizing the behavior of the function named in the subject line;
and specifically the retest of is_simple_subquery(). I've been unable
to make that fail. For example, the following patch fails to break
the regression tests:
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -718,6 +718,7 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, Ran
}
else
{
+ elog(ERROR, "croak and die");
/*
* Give up, return unmodified RangeTblRef.
*
This logic was originally introduced by the following commit:
commit e439fef6fc3e81aeb865f2c5a77c6faa2ee2a931
Author: Tom Lane
Date: Sat Jan 10 00:30:21 2004 +
Fix subquery pullup logic to not be fooled when a view that appears
'simple' references another view that is not simple. Must recheck
conditions after performing recursive pullup. Per example from
Laurent Perez, 9-Jan-04.
However, despite my best efforts, I can't figure out what scenario
it's protecting us against, at least not on current sources. The
original bug report is here:
http://archives.postgresql.org/pgsql-general/2004-01/msg00375.php
Tom's reply indicates that the v4 example shouldn't get flattened, but
it looks to me like current sources do flatten it and I really don't
see why they shouldn't. Poking around with git bisect and the patch
shown above, I see that the test case stopped tickling this code with
commit e6ae3b5dbf2c07bceb737c5a0ff199b1156051d1, which introduced
PlaceHolderVars, apparently for the precise purpose of allowing joins
of this type to be flattened. But this code survived that commit,
leaving the question of whether there are still cases where it's
needed (in which case we should probably add a comment or regression
test case, since it's not at all obvious) or whether we can rip it out
and save a few cycles.
--
Robert Haas
EnterpriseDB: 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: [HACKERS] [PATCH] PostgreSQL fails to build with 32bit MinGW-w64
On 12/05/2011 09:31 AM, NISHIYAMA Tomoaki wrote: Hi, If we are not to use 64 bit file size (and time), #undef stat may be sufficient. The #undef should be before the prototype of pgwin32_safestat because the #define stat _stat64 affect both the function and struct stat. The #undef stat necessitate #undef fstat as the parameter struct stat * is changed. I don't think I'm going to do it that way, but leave this with me, I can take it from here. Right now I'm down to the following interesting regression failure: $ cat regression.diffs *** C:/MinGW/msys/1.0/home/pgrunner/bf/root32/HEAD/pgsql/src/test/regress/expected/float8-exp-three-digits-win32.out Fri Nov 25 14:24:49 2011 --- C:/MinGW/msys/1.0/home/pgrunner/bf/root32/HEAD/pgsql/src/test/regress/results/float8.out Mon Dec 5 18:17:36 2011 *** *** 382,388 SET f1 = FLOAT8_TBL.f1 * '-1' WHERE FLOAT8_TBL.f1 > '0.0'; SELECT '' AS bad, f.f1 * '1e200' from FLOAT8_TBL f; ! ERROR: value out of range: overflow SELECT '' AS bad, f.f1 ^ '1e200' from FLOAT8_TBL f; ERROR: value out of range: overflow SELECT 0 ^ 0 + 0 ^ 1 + 0 ^ 0.0 + 0 ^ 0.5; --- 382,396 SET f1 = FLOAT8_TBL.f1 * '-1' WHERE FLOAT8_TBL.f1 > '0.0'; SELECT '' AS bad, f.f1 * '1e200' from FLOAT8_TBL f; ! bad | ?column? ! -+-- ! |0 ! | -3.484e+201 ! | -1.0043e+203 ! |-Infinity ! | -1.2345678901234 ! (5 rows) ! SELECT '' AS bad, f.f1 ^ '1e200' from FLOAT8_TBL f; ERROR: value out of range: overflow SELECT 0 ^ 0 + 0 ^ 1 + 0 ^ 0.0 + 0 ^ 0.5; == 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] why local_preload_libraries does require a separate directory ?
On 5.12.2011 00:06, Tom Lane wrote: > Tomas Vondra writes: >> On 4.12.2011 22:16, Tom Lane wrote: >>> Um ... why would you design it like that? > >> The backends are added to pg_stat_activity after the auth hook finishes, >> which means possible race conditions (backends executed at about the >> same time don't see each other in pg_stat_activity). So I use an >> exclusive lock that's acquired before reading pg_stat_activity and >> released after the pg_stat_activity is updated. >> That's the only thing the library loaded using local_preload_libraries >> does - it releases the lock. > > That's an unbelievably ugly, and dangerous, kluge. All you need is one > backend not loading the second library (and remember, > local_preload_libraries is user-settable) and you've just locked up the > system. > > Why are you using pg_stat_activity for this anyway? Searching the > ProcArray seems much safer ... see CountDBBackends for an example. Thanks, reading ProcArray works fine, although the ProcArrayStruct is private to procarray.c so I had to create a local copy. That sounds a bit too fragile I guess - whenever it changes, the extension will break without a warning. Tomas -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pg_upgrade and relkind filtering
Pg_upgrade has the following check to make sure the cluster is safe for upgrading: res = executeQueryOrDie(conn, "SELECT n.nspname, c.relname, a.attname " "FROM pg_catalog.pg_class c, " " pg_catalog.pg_namespace n, " " pg_catalog.pg_attribute a " "WHERE c.oid = a.attrelid AND " " NOT a.attisdropped AND " " a.atttypid IN ( " " 'pg_catalog.regproc'::pg_catalog.regtype, " " 'pg_catalog.regprocedure'::pg_catalog.regtype, " " 'pg_catalog.regoper'::pg_catalog.regtype, " " 'pg_catalog.regoperator'::pg_catalog.regtype, " /* regclass.oid is preserved, so 'regclass' is OK */ /* regtype.oid is preserved, so 'regtype' is OK */ " 'pg_catalog.regconfig'::pg_catalog.regtype, " " 'pg_catalog.regdictionary'::pg_catalog.regtype) AND " " c.relnamespace = n.oid AND " " n.nspname != 'pg_catalog' AND " " n.nspname != 'information_schema'"); Based on a report from EnterpriseDB, I noticed that we check all pg_class entries, while there are cases where this is unnecessary because there is no data behind the entry, e.g. views. Here are the relkinds supported: #define RELKIND_RELATION'r' /* ordinary table */ #define RELKIND_INDEX 'i' /* secondary index */ #define RELKIND_SEQUENCE'S' /* sequence object */ #define RELKIND_TOASTVALUE 't' /* for out-of-line values */ #define RELKIND_VIEW'v' /* view */ #define RELKIND_COMPOSITE_TYPE 'c' /* composite type */ #define RELKIND_FOREIGN_TABLE 'f' /* foreign table */ #define RELKIND_UNCATALOGED 'u' /* not yet cataloged */ What types, other than views, can we skip in this query? -- Bruce Momjian http://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] pg_upgrade and regclass
Bruce Momjian wrote: > Bruce Momjian wrote: > > In Postgres 8.4, pg_upgrade preserved pg_class relfilenodes by creating > > files in the file system. In Postgres 9.0, we changed this by creating > > pg_upgrade_support functions which allow us to directly preserve > > pg_class.oids. > > > > Unfortunately, check.c was not updated to reflect this and clusters > > using regclass were prevented from being upgraded by pg_upgrade. > > > > I have developed the attached patch to allow clusters using regclass to > > be upgraded. I plan to apply it to PG 9.0, 9.1, and HEAD. > > I have applied the attached patch to all relevant releases. I did a > more modest single-line code change for back branches. Oh, I forgot to mention that this bug report came to me privately via EntepriseDB testing. -- Bruce Momjian http://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] pg_upgrade and regclass
Bruce Momjian wrote: > In Postgres 8.4, pg_upgrade preserved pg_class relfilenodes by creating > files in the file system. In Postgres 9.0, we changed this by creating > pg_upgrade_support functions which allow us to directly preserve > pg_class.oids. > > Unfortunately, check.c was not updated to reflect this and clusters > using regclass were prevented from being upgraded by pg_upgrade. > > I have developed the attached patch to allow clusters using regclass to > be upgraded. I plan to apply it to PG 9.0, 9.1, and HEAD. I have applied the attached patch to all relevant releases. I did a more modest single-line code change for back branches. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + diff --git a/doc/src/sgml/pgupgrade.sgml b/doc/src/sgml/pgupgrade.sgml new file mode 100644 index 460d06b..ac3f99b *** a/doc/src/sgml/pgupgrade.sgml --- b/doc/src/sgml/pgupgrade.sgml *** psql --username postgres --file script.s *** 557,563 pg_upgrade does not support upgrading of databases containing these reg* OID-referencing system data types: regproc, regprocedure, regoper, !regoperator, regclass, regconfig, and regdictionary. (regtype can be upgraded.) --- 557,563 pg_upgrade does not support upgrading of databases containing these reg* OID-referencing system data types: regproc, regprocedure, regoper, !regoperator, regconfig, and regdictionary. (regtype can be upgraded.) diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c new file mode 100644 index d32a84c..7185f13 *** a/contrib/pg_upgrade/check.c --- b/contrib/pg_upgrade/check.c *** check_for_isn_and_int8_passing_mismatch( *** 611,621 /* * check_for_reg_data_type_usage() * pg_upgrade only preserves these system values: ! * pg_class.relfilenode * pg_type.oid * pg_enum.oid * ! * Most of the reg* data types reference system catalog info that is * not preserved, and hence these data types cannot be used in user * tables upgraded by pg_upgrade. */ --- 611,621 /* * check_for_reg_data_type_usage() * pg_upgrade only preserves these system values: ! * pg_class.oid * pg_type.oid * pg_enum.oid * ! * Many of the reg* data types reference system catalog info that is * not preserved, and hence these data types cannot be used in user * tables upgraded by pg_upgrade. */ *** check_for_reg_data_type_usage(ClusterInf *** 653,668 " NOT a.attisdropped AND " " a.atttypid IN ( " " 'pg_catalog.regproc'::pg_catalog.regtype, " ! " 'pg_catalog.regprocedure'::pg_catalog.regtype, " " 'pg_catalog.regoper'::pg_catalog.regtype, " ! " 'pg_catalog.regoperator'::pg_catalog.regtype, " ! " 'pg_catalog.regclass'::pg_catalog.regtype, " /* regtype.oid is preserved, so 'regtype' is OK */ ! " 'pg_catalog.regconfig'::pg_catalog.regtype, " ! " 'pg_catalog.regdictionary'::pg_catalog.regtype) AND " ! " c.relnamespace = n.oid AND " ! " n.nspname != 'pg_catalog' AND " ! " n.nspname != 'information_schema'"); ntups = PQntuples(res); i_nspname = PQfnumber(res, "nspname"); --- 653,668 " NOT a.attisdropped AND " " a.atttypid IN ( " " 'pg_catalog.regproc'::pg_catalog.regtype, " ! " 'pg_catalog.regprocedure'::pg_catalog.regtype, " " 'pg_catalog.regoper'::pg_catalog.regtype, " ! " 'pg_catalog.regoperator'::pg_catalog.regtype, " ! /* regclass.oid is preserved, so 'regclass' is OK */ /* regtype.oid is preserved, so 'regtype' is OK */ ! " 'pg_catalog.regconfig'::pg_catalog.regtype, " ! " 'pg_catalog.regdictionary'::pg_catalog.regtype) AND " ! " c.relnamespace = n.oid AND " ! " n.nspname != 'pg_catalog' AND " ! " n.nspname != 'information_schema'"); ntups = PQntuples(res); i_nspname = PQfnumber(res, "nspname"); -- 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] Command Triggers
Hi, Please find an update attached, v4, fixing most remaining items. Next steps are better docs and more commands support (along with finishing currently supported ones), and a review locking behavior. If you want to just scroll over the patch to get an impression of what's in there rather than try out the attachment, follow this URL: https://github.com/dimitri/postgres/compare/master...command_triggers Dimitri Fontaine writes: > Will look into qualifying names. I'm now qualifying relation names even if they have not been entered with a namespace qualifier. What do you think? The other components are left alone, I think the internal APIs for qualifying all kind of objects from the parse tree and current context are mostly missing. >> As an alternative it would be possible to pass the current search path but >> that doesn't seem to the best approach to me; > > The trigger runs with the same search_path settings as the command, right? Maybe that's good enough for command triggers? >> Command triggers should only be allowed for the database owner. > > Yes, that needs to happen soon, along with pg_dump and psql support. All three are implemented in the attached new revision of the patch. >> Imo the current callsite in ProcessUtility isn't that good. I think it would >> make more sense moving it into standard_ProcessUtility. It should be *after* >> the check_xact_readonly check in there in my opinion. > > Makes sense, will fix in the next revision. Done too. It's better this way, thank you. >> I am also don't think its a good idea to run the >> ExecBeforeOrInsteadOfCommandTriggers stuff there because thats allso the >> path >> that COMMIT/BEGIN and other stuff take. Those are pretty "fast path". >> >> I suggest making two switches in standard_ProcessUtility, one for the non- >> command trigger stuff which returns on success and one after that. Only >> after >> the first triggers are checked. > > Agreed. That's about the way I've done it. Please note that doing it this way means that a ProcessUtility_hook can decide whether or not the command triggers are going to be fired or not, and that's true for BEFORE, AFTER and INSTEAD OF command triggers. I think that's the way to go, though. >> * Either tgenable's datatype or its readfunc is wrong (watch the compiler ;)) >> * ruleutils.c: >> * generic routine for IF EXISTS, CASCADE, ... > > Will have to wait until next revision. Fixed. Well, the generic routine would only be called twice and would only share a rather short expression, so will have to wait until I add support for more commands. There's a regression tests gotcha. Namely that the parallel running of triggers against inheritance makes it impossible to predict if the trigger on the command CREATE TABLE will spit out a notice in the inherit tests. I don't know how that is usually avoided, but I guess it involves picking some other command example that don't conflict with the parallel tests of that section? Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support command-trigger.v4.patch.gz 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] hiding variable-length fields from Form_pg_* structs
On Mon, Dec 5, 2011 at 3:12 PM, Tom Lane wrote: > Robert Haas writes: >> On Mon, Dec 5, 2011 at 2:47 PM, Tom Lane wrote: >>> Peter Eisentraut writes: To clarify, I believe the rule is that the first variable-length field can be accessed as a struct field. Are there any exceptions to this? > >>> If it is known not null, yes, but I wonder just how many places actually >>> depend on that. > >> My impression is that all the varlena fields also allow nulls. > > See MARKNOTNULL in bootstrap.c. I think the exceptions were designed to > protect direct accesses to pg_index. Hmm, OK. rhaas=# select r.relname, a.attname, a.atttypid::regtype from pg_class r, pg_attribute a where relnamespace=11 and relkind='r' and attrelid = r.oid and a.attnotnull and a.attlen<0; relname | attname| atttypid +--+ pg_proc| proargtypes | oidvector pg_index | indkey | int2vector pg_index | indcollation | oidvector pg_index | indclass | oidvector pg_index | indoption| int2vector pg_trigger | tgattr | int2vector (6 rows) -- Robert Haas EnterpriseDB: 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
[HACKERS] JSON for PG 9.2
Where are we with adding JSON for Postgres 9.2? We got bogged down in the data representation last time we discussed this. -- Bruce Momjian http://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] hiding variable-length fields from Form_pg_* structs
Robert Haas writes: > On Mon, Dec 5, 2011 at 2:47 PM, Tom Lane wrote: >> Peter Eisentraut writes: >>> To clarify, I believe the rule is that the first variable-length field >>> can be accessed as a struct field. Are there any exceptions to this? >> If it is known not null, yes, but I wonder just how many places actually >> depend on that. > My impression is that all the varlena fields also allow nulls. See MARKNOTNULL in bootstrap.c. I think the exceptions were designed to protect direct accesses to pg_index. 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] pg_upgrade automatic testing
On sön, 2011-11-27 at 19:12 -0500, Andrew Dunstan wrote: > Contrib tests are only run by the buildfarm in installcheck mode, so > that will probably turn the tests off for the buildfarm, so +1 on that Does the new buildfarm modular thing support that some members could run the checks through explicit configuration? > I think these tests are probably somewhat ill-conceived. Note also > that shell scripts are not portable, and so these tests would not be > able to run on MSVC buildfarm members, even if they had been enabled in > the MSVC regression driver, which they have not. If we need a regression > driver script it needs to be written in Perl. Anyone is free to rewrite the thing in a different language. > Also note that the test as written is likely to add significantly to > buildfarm run times, as it will run the entire base regression suite > again, possibly several times. Are there any restrictions on how long a buildfarm run is supposed to take? > Finally, I think that this is probably not what we really need. What do you base your thinking on? This test suite has already found a number of bugs in the pg_upgrade procedure that no one else was able to find. By that measure, it's exactly what we need. > I have > already started work (as I mentioned some weeks ago) on having the > buildfarm stash away a successful build and data directory, to be used > later in cross-version upgrade testing, which seems to me much more what > we need from something like the buildfarm. Maybe we could discuss how to > run something like that. That is one part of the puzzle. But once you have stashed away the old source and data directory, you still need a test runner, which is exactly what this provides you. But note, cross-version pg_upgrade checks will not give you the full value, even assuming that you can make them work at all in an unattended way, because by default you won't be able to get a clean "dumps match" result, at least without a lot of additional work to mangle the dump output. Most (or all) of the bugs found so far with this test suite were for upgrades *from* whatever was the current version. If we don't have a current-to-current upgrade test suite, then we would only find those years from now. -- 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] hiding variable-length fields from Form_pg_* structs
On Mon, Dec 5, 2011 at 2:47 PM, Tom Lane wrote: > Peter Eisentraut writes: >> To clarify, I believe the rule is that the first variable-length field >> can be accessed as a struct field. Are there any exceptions to this? > > If it is known not null, yes, but I wonder just how many places actually > depend on that. It might be better to remove all varlena fields from C > visibility and require use of the accessor functions. We should at > least look into what that would cost us. My impression is that all the varlena fields also allow nulls. So I think there's no point in trying to keep the first one C-accessible. -- Robert Haas EnterpriseDB: 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: [HACKERS] hiding variable-length fields from Form_pg_* structs
Peter Eisentraut writes: > To clarify, I believe the rule is that the first variable-length field > can be accessed as a struct field. Are there any exceptions to this? If it is known not null, yes, but I wonder just how many places actually depend on that. It might be better to remove all varlena fields from C visibility and require use of the accessor functions. We should at least look into what that would cost us. > Also, this code in relcache.c accesses indclass, which is after an > int2vector and an oidvector field: > /* Check to see if it is a unique, non-partial btree index on OID */ > if (index->indnatts == 1 && > index->indisunique && index->indimmediate && > index->indkey.values[0] == ObjectIdAttributeNumber && > index->indclass.values[0] == OID_BTREE_OPS_OID && > heap_attisnull(htup, Anum_pg_index_indpred)) > oidIndex = index->indexrelid; Hmm, that does look mighty broken, doesn't it ... but somehow it works, else GetNewOid would be bleating all the time. (Thinks about that for a bit) Oh, it accidentally fails to fail because the C declarations for int2vector and oidvector are actually correct if there is a single element in the arrays, which we already verified with the indnatts test. But yeah, this seems horribly fragile, and it should not be performance critical because we only go through here when loading up a cache entry. So let's change 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] pg_upgrade automatic testing
On sön, 2011-11-27 at 18:17 -0500, Tom Lane wrote: > Peter Eisentraut writes: > > I've committed it now, and some buildfarm members are failing with lack > > of shared memory, semaphores, or disk space. Don't know what to do with > > that or why so many are failing like that. We could create a way to > > omit the test if it becomes a problem. > > I believe the issue is that those BF members have kernel settings that > only support running one postmaster at a time. The way you've got this > set up, it launches a new private postmaster during a make installcheck; > which is not only problematic from a resource consumption standpoint, > but seems to me to violate the spirit of make installcheck, because > what it's testing is not the installed postmaster but a local instance. > > Can you confine the test to only occur in "make check" mode, not "make > installcheck", please? FWIW, the original definition of installcheck is that it tests the already installed programs, which is what this does (did). But I agree that the difference is minimal in this case. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Caching for stable expressions with constant arguments v3
On 05.12.2011 21:36, Tom Lane wrote: Heikki Linnakangas writes: Or pass a flag to ExecInitExpr() to skip through the CacheExprs. Not sure what you mean by that --- are you imagining that the ExprState tree would have structure not matching the Expr tree? Yes. Or it could just set a flag in the CacheExprState nodes to not do the caching. -- Heikki Linnakangas 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] [PATCH] Caching for stable expressions with constant arguments v3
Heikki Linnakangas writes: > On 05.12.2011 20:53, Marti Raudsepp wrote: >> I considered stripping CacheExpr nodes later in PL/pgSQL, but I can't >> remember right now why I rejected that approach (sorry, it's been 2 >> months). > Yet another idea would be to leave the CacheExprs there, but provide a > way to reset the caches. PL/pgSQL could then reset the caches between > every invocation. We're likely to need a way to reset these caches anyway, at some point... > Or pass a flag to ExecInitExpr() to skip through the CacheExprs. Not sure what you mean by that --- are you imagining that the ExprState tree would have structure not matching the Expr tree? That seems just about guaranteed to break something somewhere. 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] planner fails on HEAD
Pavel Stehule writes: > 2011/12/4 Tom Lane : >> Is this x86? I can't reproduce it on x86_64. > yes, this is x86 platform > uname -a > Linux nemesis 2.6.35.14-106.fc14.i686.PAE #1 SMP Wed Nov 23 13:39:51 > UTC 2011 i686 i686 i386 GNU/Linux I reproduced this with gcc 4.6.0 on Fedora 15 x86, too. >> Also, http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45691#c4 >> indicates that an explicit cast to double should help. Would >> you check if the problem goes away if the Asserts are changed to >> >>Assert((double) outerstartsel <= (double) outerendsel); >>Assert((double) innerstartsel <= (double) innerendsel); > it doesn't help Hmm ... I'm inclined to think this actually *is* a bug, since Jakub is on record as saying it should work. Nonetheless, we need a workaround, since gcc versions behaving this way are going to be widespread for a long time even if we convince them to do something about it (which I suspect they wouldn't given their imperviousness to complaints about the main issue). I'm now thinking the best solution is just to drop these two Asserts. They're not adding anything very useful given the previous ones (which should be safe since those involve quantities rounded to integers). 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] hiding variable-length fields from Form_pg_* structs
On sön, 2011-11-27 at 18:20 -0500, Tom Lane wrote:
> The low-tech way would be
>
> CATALOG(pg_attribute,1249) BKI_BOOTSTRAP ...
> {
> ...
> int4attinhcount;
> Oid attcollation;
> aclitem attacl[1];
> #ifdef CATALOG_VARLEN_FIELDS
> textattoptions[1];
> textattfdwoptions[1];
> #endif
> } FormData_pg_attribute;
Good enough.
To clarify, I believe the rule is that the first variable-length field
can be accessed as a struct field. Are there any exceptions to this?
This kind of comment is pretty confusing:
CATALOG(pg_rewrite,2618)
{
NameDatarulename;
Oid ev_class;
int2ev_attr;
charev_type;
charev_enabled;
boolis_instead;
/* NB: remaining fields must be accessed via heap_getattr */
pg_node_tree ev_qual;
pg_node_tree ev_action;
} FormData_pg_rewrite;
Also, this code in relcache.c accesses indclass, which is after an
int2vector and an oidvector field:
/* Check to see if it is a unique, non-partial btree index on OID */
if (index->indnatts == 1 &&
index->indisunique && index->indimmediate &&
index->indkey.values[0] == ObjectIdAttributeNumber &&
index->indclass.values[0] == OID_BTREE_OPS_OID &&
heap_attisnull(htup, Anum_pg_index_indpred))
oidIndex = index->indexrelid;
--
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: [PATCH] Caching for stable expressions with constant arguments v3
Heikki Linnakangas writes: > I wonder if it would be better to add the CacheExpr nodes to the tree as > a separate pass, instead of shoehorning it into eval_const_expressions? > I think would be more readable that way, even though a separate pass > would be more expensive. A separate pass would be very considerably more expensive, because (1) it would require making a whole new copy of each expression tree, and (2) it would require looking up the volatility status of each function and operator. eval_const_expressions already has to do the latter, or has to do it in a lot of cases anyway, so I think it's probably the best place to add this. If it weren't for (2) I would suggest adding the work to setrefs.c instead, but as it is I think we'd better suck it up and deal with any fallout in the later stages of the planner. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Caching for stable expressions with constant arguments v3
On 05.12.2011 20:53, Marti Raudsepp wrote: I considered stripping CacheExpr nodes later in PL/pgSQL, but I can't remember right now why I rejected that approach (sorry, it's been 2 months). Yet another idea would be to leave the CacheExprs there, but provide a way to reset the caches. PL/pgSQL could then reset the caches between every invocation. Or pass a flag to ExecInitExpr() to skip through the CacheExprs. -- Heikki Linnakangas 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] [PATCH] Caching for stable expressions with constant arguments v3
On Mon, Dec 5, 2011 at 19:31, Tom Lane wrote: > I think if you have some call sites inject CacheExprs and others not, > it will get more difficult to match up expressions that should be > considered equal. On the whole this seems like a bad idea. What is > the reason for having such a control boolean in the first place? It's needed for correctness with PL/pgSQL simple expressions. This seems a bit of a kludge, but I considered it the "least bad" solution. Here's what I added to planner.c standard_planner(): /* * glob->isSimple is a hint to eval_const_expressions() and PL/pgSQL that * this statement is potentially a simple expression -- it contains no * table references, no subqueries and no join clauses. * * We need this here because this prevents insertion of CacheExpr, which * would break simple expressions in PL/pgSQL. Such queries wouldn't * benefit from constant caching anyway. * * The actual definition of a simple statement is more strict, but we * don't want to spend that checking overhead here. * * Caveat: Queries with set-returning functions in SELECT list could * still potentially benefit from caching, but we don't check that now. */ glob->isSimple = (parse->commandType == CMD_SELECT && parse->jointree->fromlist == NIL && parse->hasSubLinks == FALSE && parse->cteList == NIL); I considered stripping CacheExpr nodes later in PL/pgSQL, but I can't remember right now why I rejected that approach (sorry, it's been 2 months). Currently I'm also disabling CacheExpr nodes in estimate_expression_value() since we know for a fact that the planner only evaluates it once. But that probably doesn't make much of a difference. Regards, Marti -- 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] planner fails on HEAD
2011/12/5 Merlin Moncure : > On Mon, Dec 5, 2011 at 12:17 PM, Pavel Stehule > wrote: >> Hello >> >> 2011/12/4 Tom Lane : >>> Pavel Stehule writes: it looks like gcc bug - gcc 4.5.1 20100924 (Red Hat 4.5.1) It was configured just with --enable-debug and --enable-cassert >>> >>> Is this x86? I can't reproduce it on x86_64. >>> >> >> yes, this is x86 platform >> >> uname -a >> Linux nemesis 2.6.35.14-106.fc14.i686.PAE #1 SMP Wed Nov 23 13:39:51 >> UTC 2011 i686 i686 i386 GNU/Linux >> >> [pavel@nemesis ~]$ cat /proc/cpuinfo >> processor : 0 >> vendor_id : GenuineIntel >> cpu family : 6 >> model : 15 >> model name : Intel(R) Core(TM)2 Duo CPU T7700 @ 2.40GHz >> stepping : 11 >> cpu MHz : 800.000 >> cache size : 4096 KB >> physical id : 0 >> siblings : 2 >> core id : 0 >> cpu cores : 2 >> apicid : 0 >> initial apicid : 0 >> fdiv_bug : no >> hlt_bug : no >> f00f_bug : no >> coma_bug : no >> fpu : yes >> fpu_exception : yes >> cpuid level : 10 >> wp : yes >> flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca >> cmov >> pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe nx lm >> constant_tsc arch_perfmon pebs bts aperfmperf pni dtes64 monitor >> ds_cpl vmx est tm2 ssse3 cx16 xtpr pdcm lahf_lm ida tpr_shadow vnmi >> flexpriority >> bogomips : 4785.76 >> clflush size : 64 >> cache_alignment : 64 >> address sizes : 36 bits physical, 48 bits virtual >> power management: >> >> processor : 1 >> vendor_id : GenuineIntel >> cpu family : 6 >> model : 15 >> model name : Intel(R) Core(TM)2 Duo CPU T7700 @ 2.40GHz >> stepping : 11 >> cpu MHz : 800.000 >> cache size : 4096 KB >> physical id : 0 >> siblings : 2 >> core id : 1 >> cpu cores : 2 >> apicid : 1 >> initial apicid : 1 >> fdiv_bug : no >> hlt_bug : no >> f00f_bug : no >> coma_bug : no >> fpu : yes >> fpu_exception : yes >> cpuid level : 10 >> wp : yes >> flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca >> cmov >> pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe nx lm >> constant_tsc arch_perfmon pebs bts aperfmperf pni dtes64 monitor >> ds_cpl vmx est tm2 ssse3 cx16 xtpr pdcm lahf_lm ida tpr_shadow vnmi >> flexpriority >> bogomips : 4786.60 >> clflush size : 64 >> cache_alignment : 64 >> address sizes : 36 bits physical, 48 bits virtual >> power management: >> >> it is Dell latitude D830 >> >>> It's fairly easy to get a set of values such that innerstartsel *should* >>> equal innerendsel; but if one value has been rounded to memory precision >>> and the other hasn't, the assert could certainly fail. >>> >>> Some digging around yields the information that the gcc hackers do not >>> consider this a bug, or at least adamantly refuse to do anything about it: >>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=323 >>> Comment 47 is particularly relevant to our situation: >>> >>> To summarize, this defect effectively states that: >>> assert( (x/y) == (x/y) ) >>> may cause an assertion if compiled with optimization. >>> >>> Also, http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45691#c4 >>> indicates that an explicit cast to double should help. Would >>> you check if the problem goes away if the Asserts are changed to >>> >>> Assert((double) outerstartsel <= (double) outerendsel); >>> Assert((double) innerstartsel <= (double) innerendsel); >>> >> >> it doesn't help >> >>> regards, tom lane >> >> assambler list is attached > > how about: > Assert((volatile double) outerstartsel <= (volatile double) outerendsel); doesn't help too Regards Pavel > etc > > 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] planner fails on HEAD
On Mon, Dec 5, 2011 at 12:17 PM, Pavel Stehule wrote: > Hello > > 2011/12/4 Tom Lane : >> Pavel Stehule writes: >>> it looks like gcc bug - gcc 4.5.1 20100924 (Red Hat 4.5.1) It was >>> configured just with --enable-debug and --enable-cassert >> >> Is this x86? I can't reproduce it on x86_64. >> > > yes, this is x86 platform > > uname -a > Linux nemesis 2.6.35.14-106.fc14.i686.PAE #1 SMP Wed Nov 23 13:39:51 > UTC 2011 i686 i686 i386 GNU/Linux > > [pavel@nemesis ~]$ cat /proc/cpuinfo > processor : 0 > vendor_id : GenuineIntel > cpu family : 6 > model : 15 > model name : Intel(R) Core(TM)2 Duo CPU T7700 @ 2.40GHz > stepping : 11 > cpu MHz : 800.000 > cache size : 4096 KB > physical id : 0 > siblings : 2 > core id : 0 > cpu cores : 2 > apicid : 0 > initial apicid : 0 > fdiv_bug : no > hlt_bug : no > f00f_bug : no > coma_bug : no > fpu : yes > fpu_exception : yes > cpuid level : 10 > wp : yes > flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca > cmov > pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe nx lm > constant_tsc arch_perfmon pebs bts aperfmperf pni dtes64 monitor > ds_cpl vmx est tm2 ssse3 cx16 xtpr pdcm lahf_lm ida tpr_shadow vnmi > flexpriority > bogomips : 4785.76 > clflush size : 64 > cache_alignment : 64 > address sizes : 36 bits physical, 48 bits virtual > power management: > > processor : 1 > vendor_id : GenuineIntel > cpu family : 6 > model : 15 > model name : Intel(R) Core(TM)2 Duo CPU T7700 @ 2.40GHz > stepping : 11 > cpu MHz : 800.000 > cache size : 4096 KB > physical id : 0 > siblings : 2 > core id : 1 > cpu cores : 2 > apicid : 1 > initial apicid : 1 > fdiv_bug : no > hlt_bug : no > f00f_bug : no > coma_bug : no > fpu : yes > fpu_exception : yes > cpuid level : 10 > wp : yes > flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca > cmov > pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe nx lm > constant_tsc arch_perfmon pebs bts aperfmperf pni dtes64 monitor > ds_cpl vmx est tm2 ssse3 cx16 xtpr pdcm lahf_lm ida tpr_shadow vnmi > flexpriority > bogomips : 4786.60 > clflush size : 64 > cache_alignment : 64 > address sizes : 36 bits physical, 48 bits virtual > power management: > > it is Dell latitude D830 > >> It's fairly easy to get a set of values such that innerstartsel *should* >> equal innerendsel; but if one value has been rounded to memory precision >> and the other hasn't, the assert could certainly fail. >> >> Some digging around yields the information that the gcc hackers do not >> consider this a bug, or at least adamantly refuse to do anything about it: >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=323 >> Comment 47 is particularly relevant to our situation: >> >> To summarize, this defect effectively states that: >> assert( (x/y) == (x/y) ) >> may cause an assertion if compiled with optimization. >> >> Also, http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45691#c4 >> indicates that an explicit cast to double should help. Would >> you check if the problem goes away if the Asserts are changed to >> >> Assert((double) outerstartsel <= (double) outerendsel); >> Assert((double) innerstartsel <= (double) innerendsel); >> > > it doesn't help > >> regards, tom lane > > assambler list is attached how about: Assert((volatile double) outerstartsel <= (volatile double) outerendsel); etc 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] plpython SPI cursors
On 05/12/11 19:12, Bruce Momjian wrote: > Jan Urbański wrote: >> On 05/12/11 18:58, Peter Eisentraut wrote: >>> On ons, 2011-11-23 at 19:58 +0100, Jan Urba?ski wrote: On 20/11/11 19:14, Steve Singer wrote: Responding now to all questions and attaching a revised patch based on your comments. >>> >>> Committed >>> >>> Please refresh the other patch. >> >> Great, thanks! >> >> I'll try to send an updated version of the other patch this evening. > > I assume this is _not_ related to this TODO item: > > Add a DB-API compliant interface on top of the SPI interface No, not related. -- 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] plpython SPI cursors
Jan Urba??ski wrote: > On 05/12/11 18:58, Peter Eisentraut wrote: > > On ons, 2011-11-23 at 19:58 +0100, Jan Urba?ski wrote: > >> On 20/11/11 19:14, Steve Singer wrote: > >> Responding now to all questions and attaching a revised patch based on > >> your comments. > > > > Committed > > > > Please refresh the other patch. > > Great, thanks! > > I'll try to send an updated version of the other patch this evening. I assume this is _not_ related to this TODO item: Add a DB-API compliant interface on top of the SPI interface -- Bruce Momjian http://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] exit() calls in libraries
Excerpts from Peter Eisentraut's message of lun dic 05 14:27:41 -0300 2011: > The cases in libpq are > > * various places in fe-print.c calling exit(1) when malloc fails, > presumably having run out of memory, and > * in libpq-int.h the macro PGTHREAD_ERROR, which is called in > several places in fe-connect.c and fe-secure.c. > > Are these appropriate behaviors? The fe-print.c stuff probably isn't > used much anymore. But the threading stuff is, and it encroaches on the > exit status space of the libpq-using program. And does it even make > sense to call exit() if the thread locking is busted? Maybe abort() > would work better? Having had to battle some exit() calls in the PHP interpreter back when I was working in PL/php, I agree that they shouldn't be there -- abort() seems more appropriate if the system is truly busted. As for the fr-print.c code, I'm not really sure why don't we just remove it. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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] plpython SPI cursors
On 05/12/11 18:58, Peter Eisentraut wrote: > On ons, 2011-11-23 at 19:58 +0100, Jan Urbański wrote: >> On 20/11/11 19:14, Steve Singer wrote: >> Responding now to all questions and attaching a revised patch based on >> your comments. > > Committed > > Please refresh the other patch. Great, thanks! I'll try to send an updated version of the other patch this evening. Cheers, Jan -- 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] plpython SPI cursors
On ons, 2011-11-23 at 19:58 +0100, Jan Urbański wrote: > On 20/11/11 19:14, Steve Singer wrote: > > On 11-10-15 07:28 PM, Jan Urbański wrote: > >> Hi, > >> > >> attached is a patch implementing the usage of SPI cursors in PL/Python. > >> Currently when trying to process a large table in PL/Python you have > >> slurp it all into memory (that's what plpy.execute does). > >> > >> J > > > > I found a few bugs (see my testing section below) that will need fixing > > + a few questions about the code > > Responding now to all questions and attaching a revised patch based on > your comments. Committed Please refresh the other patch. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Inlining comparators as a performance optimisation
Peter Geoghegan writes: > Why the strong aversion to what I've done? I accept that it's ugly, > but is it really worth worrying about that sort of regression in > maintainability for something that was basically untouched since 2006, > and will probably remain untouched after this work concludes, whatever > happens? Maintainability is only part of the issue --- though it's definitely one part, since this code has hardly gone "untouched since 2006", cf http://git.postgresql.org/gitweb/?p=postgresql.git;a=history;f=src/backend/utils/sort/tuplesort.c;hb=HEAD What is bothering me is that this approach is going to cause substantial bloat of the executable code, and such bloat has got distributed costs, which we don't have any really good way to measure but for sure micro-benchmarks addressing only sort speed aren't going to reveal them. Cache lines filled with sort code take cycles to flush and replace with something else. I think it's possibly reasonable to have inlined copies of qsort for a small number of datatypes, but it seems much less reasonable to have multiple copies per datatype in order to obtain progressively tinier micro-benchmark speedups. We need to figure out what the tradeoff against executable size really is, but so far it seems like you've been ignoring the fact that there is any such tradeoff at all. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Caching for stable expressions with constant arguments v3
Marti Raudsepp writes: > On Sun, Dec 4, 2011 at 22:53, Heikki Linnakangas > wrote: >> This comment in RelationGetExpressions() also worries me: > [...] >> Do the injected CacheExprs screw up that equality? Or the constraint >> exclusion logic in predtest.c? > I suspect these cases are guaranteed not to produce any CacheExprs. > They're always immutable expressions. If they contain Var references > they're stored as is (not cachable); if not, they're folded to a > constant. > But I will have to double-check all the callers; it might be a good > idea to disable caching anyway in these cases. I think if you have some call sites inject CacheExprs and others not, it will get more difficult to match up expressions that should be considered equal. On the whole this seems like a bad idea. What is the reason for having such a control boolean in the first place? 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] exit() calls in libraries
Debian's package policy and quality check tool lintian reports the following (among other things) on the postgresql-9.1 (and earlier) packages: X: libpq5: shlib-calls-exit usr/lib/libpq.so.5.4 X: libecpg6: shlib-calls-exit usr/lib/libecpg.so.6.3 which is explained as I: shlib-calls-exit N: N: The listed shared library calls the C library exit() or _exit() N: functions. N: N: In the case of an error, the library should instead return an N: appropriate error code to the calling program which can then determine N: how to handle the error, including performing any required clean-up. [snip] The report on libecpg is actually a false positive, because the exit() call comes from get_progname() in path.c, which is not called from functions in libecpg. The cases in libpq are * various places in fe-print.c calling exit(1) when malloc fails, presumably having run out of memory, and * in libpq-int.h the macro PGTHREAD_ERROR, which is called in several places in fe-connect.c and fe-secure.c. Are these appropriate behaviors? The fe-print.c stuff probably isn't used much anymore. But the threading stuff is, and it encroaches on the exit status space of the libpq-using program. And does it even make sense to call exit() if the thread locking is busted? Maybe abort() would work better? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Caching for stable expressions with constant arguments v3
On Sun, Dec 4, 2011 at 22:53, Heikki Linnakangas wrote: > This seems to have bitrotted, thanks to the recent refactoring in > eval_const_expressions(). Luckily the conflicts are mostly whitespace changes, so shouldn't be hard to fix. I'll try to come up with an updated patch today or tomorrow. > I wonder if it would be better to add the CacheExpr nodes to the tree as a > separate pass, instead of shoehorning it into eval_const_expressions? I > think would be more readable that way, even though a separate pass would be > more expensive. And there are callers of eval_const_expressions() that have > no use for the caching, like process_implied_equality(). Per Tom's comment, I won't split out the cache insertion for now. The context struct has a boolean 'cache' attribute that controls whether caching is desired or not. I think this could be exposed to the caller as an eval_const_expressions() argument. > This comment in RelationGetExpressions() also worries me: [...] > Do the injected CacheExprs screw up that equality? Or the constraint > exclusion logic in predtest.c? I suspect these cases are guaranteed not to produce any CacheExprs. They're always immutable expressions. If they contain Var references they're stored as is (not cachable); if not, they're folded to a constant. But I will have to double-check all the callers; it might be a good idea to disable caching anyway in these cases. Regards, Marti -- 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] Inlining comparators as a performance optimisation
On 5 December 2011 13:23, Heikki Linnakangas wrote: > I'm still not convinced the big gain is in inlining the comparison > functions. Your patch contains a few orthogonal tricks, too: > > * A fastpath for the case of just one scankey. No reason we can't do that > without inlining. True, though Tom did seem to not like that idea very much. > * inlining the swap-functions within qsort. Thaẗ́'s different from inlining > the comparison functions, and could be done regardless of the data type. The > qsort element size in tuplesort.c is always sizeof(SortTuple) Sure. I think that Tom mostly objects to hard-coded intelligence about which datatypes are used, rather than specialisations per se. > And there's some additional specializations we can do, not implemented in > your patch, that don't depend on inlining the comparison functions: > > * A fastpath for the case of no NULLs > * separate comparetup functions for ascending and descending sorts (this > allows tail call elimination of the call to type-specific comparison > function in the ascending version) > * call CHECK_FOR_INTERRUPTS() less frequently. All of those had occurred to me, except the last - if you look at the definition of that macro, it looks like more trouble than it's worth to do less frequently. I didn't revise my patch with them though, because the difference that they made, while clearly measurable, seemed fairly small, or at least relatively so. I wasn't about to add additional kludge for marginal benefits given the existing controversy, though I have not dismissed the idea entirely - it could be important on some other machine. > Perhaps you can get even more gain by adding the no-NULLs, and asc/desc > special cases to your inlining patch, too, but let's squeeze all the fat out > of the non-inlined version first. As I said, those things are simple enough to test, and were not found to make a significant difference, at least to my exact test case + hardware. > One nice aspect of many of these > non-inlining optimizations is that they help with external sorts, too. I'd expect the benefits to be quite a lot smaller there, but fair point. Results from running the same test on your patch are attached. I think that while you're right to suggest that the inlining of the comparator proper, rather than any other function or other optimisation isn't playing a dominant role in these optimisations, I think that you're underestimating the role of locality of reference. To illustrate this, I've also included results for when I simply move the comparator to another translation unit, logtape.c . There's clearly a regression from doing so (I ran the numbers twice, in a clinical environment). The point is, there is a basically unknown overhead paid for not inlining - maybe inlining is not worth it, but that's a hard call to make. I didn't try to make the numbers look worse by putting the comparator proper in some other translation unit, but it may be that I'd have shown considerably worse numbers by doing so. Why the strong aversion to what I've done? I accept that it's ugly, but is it really worth worrying about that sort of regression in maintainability for something that was basically untouched since 2006, and will probably remain untouched after this work concludes, whatever happens? -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services results_server_w_heikki.ods Description: application/vnd.oasis.opendocument.spreadsheet -- 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] [REVIEW] Patch for cursor calling with named parameters
"Kevin Grittner" wrote: > Yeb Havinga wrote: >> I personally tend to believe it doesn't even need to be an error. >> There is no technical reason not to allow it. All the user needs >> to do is make sure that the combination of named parameters and >> the positional ones together are complete and not overlapping. >> This is also in line with calling functions, where mixing named >> and positional is allowed, as long as the positional arguments >> are first. Personally I have no opinion what is best, include the >> feature or give an error, and I removed the possibility during >> the previous commitfest. > > If there's no technical reason not to allow them to be mixed, I > would tend to favor consistency with parameter calling rules. > Doing otherwise seems like to result in confusion and "bug" > reports. Er, that was supposed to read "I would tend to favor consistency with function calling rules." As stated here: http://www.postgresql.org/docs/9.1/interactive/sql-syntax-calling-funcs.html | PostgreSQL also supports mixed notation, which combines positional | and named notation. In this case, positional parameters are | written first and named parameters appear after them. > How do others feel? If there are no objections, I suggest that Yeb implement the mixed notation for cursor parameters. -Kevin -- 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] cannot read pg_class without having selected a database / is this a bug?
On Mon, Dec 5, 2011 at 10:46 AM, Tom Lane wrote: > Robert Haas writes: >> I'm still puzzled that Tomas got it working at all. If MyDatabaseId >> hasn't been set yet, the how did we manage to build a relcache entry >> for anything - let alone an unshared catalog? > > Well, he wasn't actually issuing a SQL query, just calling some of the > pgstat.c subroutines that underlie the view. It happens that the pgstat > module has no backend-local initialization (at the moment, and > discounting the issue of making the process's own pgstat_activity entry), > so they were happy enough. It was the syscache stuff that was spitting > up. Oh, I see. -- Robert Haas EnterpriseDB: 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: [HACKERS] missing rename support
Robert Haas writes: > I don't think so. There's no ALTER RULE command; should we add one > (matching ALTER TRIGGER) or make this part of ALTER TABLE? I don't > think constraints can be renamed either, which should probably be > addressed along with rules. Note that renaming an index-based constraint should also rename the index. I believe the other direction works already. 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] cannot read pg_class without having selected a database / is this a bug?
Robert Haas writes: > I'm still puzzled that Tomas got it working at all. If MyDatabaseId > hasn't been set yet, the how did we manage to build a relcache entry > for anything - let alone an unshared catalog? Well, he wasn't actually issuing a SQL query, just calling some of the pgstat.c subroutines that underlie the view. It happens that the pgstat module has no backend-local initialization (at the moment, and discounting the issue of making the process's own pgstat_activity entry), so they were happy enough. It was the syscache stuff that was spitting up. 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] missing rename support
On Sat, Dec 3, 2011 at 4:46 PM, Peter Eisentraut wrote: > I noticed the following object types don't have support for an ALTER ... > RENAME command: > > DOMAIN (but ALTER TYPE works) > FOREIGN DATA WRAPPER > OPERATOR > RULE > SERVER > > Are there any restrictions why these couldn't be added? I don't think so. There's no ALTER RULE command; should we add one (matching ALTER TRIGGER) or make this part of ALTER TABLE? I don't think constraints can be renamed either, which should probably be addressed along with rules. -- Robert Haas EnterpriseDB: 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: [HACKERS] cannot read pg_class without having selected a database / is this a bug?
On Sun, Dec 4, 2011 at 4:26 PM, Tom Lane wrote: > Tomas Vondra writes: >> What about the pg_stat_activity - is it safe to access that from auth >> hook or is that just a coincidence that it works (and might stop working >> in the future)? > > It doesn't seem like a good thing to rely on. There's certainly no > testing being done that would cause us to notice if it stopped working > so early in backend startup. I'm still puzzled that Tomas got it working at all. If MyDatabaseId hasn't been set yet, the how did we manage to build a relcache entry for anything - let alone an unshared catalog? -- Robert Haas EnterpriseDB: 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: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
Yeb Havinga wrote: > On 2011-12-03 21:53, Kevin Grittner wrote: >> (1) Doc changes: >> >>(a) These contain some unnecessary whitespace changes which >>make it harder to see exactly what changed. > > This is probably caused because I used emacs's fill-paragraph > (alt+q) command, after some changes. If this is against policy, I > could change the additions in such a way as to cause minor > differences, however I believe that the benefit of that ends > immediately after review. I don't know whether it's a hard policy, but people usually minimize whitespace changes in such situations, to make it easier for the reviewer and committer to tell what really changed. The committer can always reformat after looking that over, if they feel that's useful. The issue is small enough here that I'm not inclined to consider it a blocker for sending to the committer. >> (2) The error for mixing positional and named parameters should >> be moved up. That seems more important than "too many arguments" >> or "provided multiple times" if both are true. > > I moved the error up, though I personally tend to believe it > doesn't even need to be an error. There is no technical reason not > to allow it. All the user needs to do is make sure that the > combination of named parameters and the positional ones together > are complete and not overlapping. This is also in line with > calling functions, where mixing named and positional is allowed, > as long as the positional arguments are first. Personally I have > no opinion what is best, include the feature or give an error, and > I removed the possibility during the previous commitfest. If there's no technical reason not to allow them to be mixed, I would tend to favor consistency with parameter calling rules. Doing otherwise seems like to result in confusion and "bug" reports. How do others feel? -Kevin -- 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] planner fails on HEAD
Merlin Moncure writes: > On Sun, Dec 4, 2011 at 4:55 PM, Tom Lane wrote: >> Pavel Stehule writes: > it looks like gcc bug - gcc 4.5.1 20100924 (Red Hat 4.5.1) It was > configured just with --enable-debug and --enable-cassert >> >> Is this x86? I can't reproduce it on x86_64. > reading all the comments in the gcc bug report, this is because x86 > targets the x87 fpu by default which is where the bug is -- it's a > hardware problem. x86_64 targets sse which has stricter standards for > rounding. most x86 processors support sse -- is there a reason why we > don't target sse? Well, older machines won't have sse, and in any case I think x86 is not the only architecture with the issue, just the most popular one. Floating-point registers that are wider than standard double are hardly an unusual idea. 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] Command Triggers
On Sat, Dec 03, 2011 at 01:26:22AM +0100, Andres Freund wrote: > On Saturday, December 03, 2011 01:09:48 AM Alvaro Herrera wrote: > > Excerpts from Andres Freund's message of vie dic 02 19:09:47 -0300 2011: > > > Hi all, > > > > > > There is also the point about how permission checks on the actual > > > commands (in comparison of modifying command triggers) and such are > > > handled: > > > > > > BEFORE and INSTEAD will currently be called independently of the fact > > > whether the user is actually allowed to do said action (which is > > > inconsistent with data triggers) and indepentent of whether the object > > > they concern exists. > > > > > > I wonder if anybody considers that a problem? > > > > Hmm, we currently even have a patch (or is it already committed?) to > > avoid locking objects before we know the user has permission on the > > object. Getting to the point of calling the trigger would surely be > > even worse. > Well, calling the trigger won't allow them to lock the object. It doesn't > even > confirm the existance of the table. > didn't I see a discussion in passing about the possibility of using these command triggers to implement some aspects of se-pgsql? In that case, you'd need the above behavior. Ross -- Ross Reedstrom, Ph.D. reeds...@rice.edu Systems Engineer & Admin, Research Scientistphone: 713-348-6166 Connexions http://cnx.orgfax: 713-348-3665 Rice University MS-375, Houston, TX 77005 GPG Key fingerprint = F023 82C8 9B0E 2CC6 0D8E F888 D3AE 810E 88F0 BEDE -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] PostgreSQL fails to build with 32bit MinGW-w64
Hi, If we are not to use 64 bit file size (and time), #undef stat may be sufficient. The #undef should be before the prototype of pgwin32_safestat because the #define stat _stat64 affect both the function and struct stat. The #undef stat necessitate #undef fstat as the parameter struct stat * is changed. Additional change are for the macro redefinition warnings. (Suppress warnings, but perhaps not very different) The patch is tested to compile on x86_64-w64-mingw32-gcc 4.7.0 20111203 (experimental) and gcc version 4.6.1 on MingW/MSYS --- a/src/include/port.h +++ b/src/include/port.h @@ -334,6 +334,12 @@ extern bool rmtree(const char *path, bool rmtopdir); */ #if defined(WIN32) && !defined(__CYGWIN__) && !defined(UNSAFE_STAT_OK) #include +#ifdef stat +#undef stat +#endif +#ifdef fstat +#undef fstat +#endif extern int pgwin32_safestat(const char *path, struct stat * buf); #define stat(a,b) pgwin32_safestat(a,b) If this is not sufficient, we might need to change all call of stat, lstat, and fstat to some wrapper functions? : It's theoretically doable, but could be quite difficult for a huge software. pgsql-mingw64-patch.diff Description: Binary data On 2011/12/05, at 1:10, Andrew Dunstan wrote: > > > On 12/04/2011 06:31 AM, Magnus Hagander wrote: >> On Sun, Dec 4, 2011 at 09:14, NISHIYAMA Tomoaki >> wrote: >>> Hi, >>> >>> I found error on #define stat _stat64 occurs on Mingw-w64 >>> (x86_64-w64-mingw32) >>> gcc version 4.7.0 20111203 (experimental) (GCC) >>> >>> The code can be compiled with >>> >>> diff --git a/src/include/port.h b/src/include/port.h >>> index eceb4bf..78d5c92 100644 >>> --- a/src/include/port.h >>> +++ b/src/include/port.h >>> @@ -332,7 +332,7 @@ extern bool rmtree(const char *path, bool rmtopdir); >>> * Some frontends don't need the size from stat, so if UNSAFE_STAT_OK >>> * is defined we don't bother with this. >>> */ >>> -#if defined(WIN32)&& !defined(__CYGWIN__)&& !defined(UNSAFE_STAT_OK) >>> +#if defined(WIN32_ONLY_COMPILER)&& !defined(UNSAFE_STAT_OK) >>> #include >>> extern int pgwin32_safestat(const char *path, struct stat * buf); >>> >>> but, surely we need to know if it is ok or not >>> as the comment before says: >>> * stat() is not guaranteed to set the st_size field on win32, so we >>> * redefine it to our own implementation that is. >>> >>> Is there any simple test program that determines if the pgwin32_safestat >>> is required or the library stat is sufficient? >>> I presume the stat is a library function and therefore it depends on the >>> compiler rather than the WIN32 platform as a whole. >> No, stat() is unreliable because it is implemented on top of >> FindNextFile(), and *that's* where the actual problem is at. And >> that's an OS API function, not a library function. See the discussion >> at http://archives.postgresql.org/pgsql-hackers/2008-03/msg01181.php >> >> In theory, if mingw implemented their stat() without using >> FindNextFile(), it might work - but I don't see how they'd do it in >> that case. And I can't see us going into details to remove such a >> simple workaround even if they do - it's better to ensure we work the >> same way with different compilers. >> > > > Yeah. > > > This is only a problem because, with this compiler, configure finds this: > > checking for _FILE_OFFSET_BITS value needed for large files... 64 > checking size of off_t... 8 > > whereas on the mingw-w64 compiler pitta is using it finds this: > > checking for _FILE_OFFSET_BITS value needed for large files... unknown > checking for _LARGE_FILES value needed for large files... unknown > checking size of off_t... 4 > > > It's the setting of _FILE_OFFSET_BITS that causes the offending macro > definition. > > Can we just turn off largefile support for this compiler, or maybe for all > mingw builds, possibly by just disabling the checks in configure.in? I note > it's turned off for MSVC in all flavors apparently. pgwin32_safestat() isn't > safe for large files anyway, so there would be good grounds for doing so > quite apart from this, ISTM. (Of course, we should work out how to handle > large files properly on Windows, but that's a task for another day.) > > (BTW, someone asked me the other day why anyone would want to do 32 bit > builds. One answer is that often the libraries you want to link with are only > available in 32 bit versions.) > > > 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 > -- 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] planner fails on HEAD
On Sun, Dec 4, 2011 at 4:55 PM, Tom Lane wrote: > Pavel Stehule writes: >> it looks like gcc bug - gcc 4.5.1 20100924 (Red Hat 4.5.1) It was >> configured just with --enable-debug and --enable-cassert > > Is this x86? I can't reproduce it on x86_64. reading all the comments in the gcc bug report, this is because x86 targets the x87 fpu by default which is where the bug is -- it's a hardware problem. x86_64 targets sse which has stricter standards for rounding. most x86 processors support sse -- is there a reason why we don't target sse? 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] Inlining comparators as a performance optimisation
On 05.12.2011 02:14, Peter Geoghegan wrote:
On 4 December 2011 19:17, Tom Lane wrote:
I have not done any performance testing on this patch, but it might be
interesting to check it with the same test cases Peter's been using.
I've attached a revision of exactly the same benchmark run to get the
results in results_server.ods .
You'll see very similar figures to results_server.ods for HEAD and for
my patch, as you'd expect. I think the results speak for themselves. I
maintain that we should use specialisations - that's where most of the
benefit is to be found.
I'm still not convinced the big gain is in inlining the comparison
functions. Your patch contains a few orthogonal tricks, too:
* A fastpath for the case of just one scankey. No reason we can't do
that without inlining.
* inlining the swap-functions within qsort. Thaẗ́'s different from
inlining the comparison functions, and could be done regardless of the
data type. The qsort element size in tuplesort.c is always sizeof(SortTuple)
And there's some additional specializations we can do, not implemented
in your patch, that don't depend on inlining the comparison functions:
* A fastpath for the case of no NULLs
* separate comparetup functions for ascending and descending sorts (this
allows tail call elimination of the call to type-specific comparison
function in the ascending version)
* call CHECK_FOR_INTERRUPTS() less frequently.
To see how much difference those things make, I hacked together the
attached patch. I didn't base this on Robert's/Tom's patch, but instead
just added a quick & dirty FunctionCallInvoke-overhead-skipping version
of btint4cmp. I believe that aspect of this patch it's similar in
performance characteristics, though.
In my quick tests, it gets quite close in performance to your inlining
patch, when sorting int4s and the input contains no NULLs. But please
give it a try in your test environment, to get numbers comparable with
your other tests.
Perhaps you can get even more gain by adding the no-NULLs, and asc/desc
special cases to your inlining patch, too, but let's squeeze all the fat
out of the non-inlined version first. One nice aspect of many of these
non-inlining optimizations is that they help with external sorts, too.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index 3505236..85bec20 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -114,6 +114,8 @@
#include "utils/rel.h"
#include "utils/tuplesort.h"
+#include "utils/builtins.h"
+
/* sort-type codes for sort__start probes */
#define HEAP_SORT 0
@@ -273,6 +275,8 @@ struct Tuplesortstate
int memtupcount; /* number of tuples currently present */
int memtupsize; /* allocated length of memtuples array */
+ bool hasnulls_initial; /* any NULLs in the first key col? */
+
/*
* While building initial runs, this is the current output run number
* (starting at 0). Afterwards, it is the number of initial runs we made.
@@ -341,6 +345,8 @@ struct Tuplesortstate
TupleDesc tupDesc;
ScanKey scanKeys; /* array of length nKeys */
+ int (*comparator) (Datum, Datum);
+
/*
* These variables are specific to the CLUSTER case; they are set by
* tuplesort_begin_cluster. Note CLUSTER also uses tupDesc and
@@ -459,6 +465,11 @@ static unsigned int getlen(Tuplesortstate *state, int tapenum, bool eofOK);
static void markrunend(Tuplesortstate *state, int tapenum);
static int comparetup_heap(const SortTuple *a, const SortTuple *b,
Tuplesortstate *state);
+static int comparetup_heap_1key_asc_nonulls(const SortTuple *a, const SortTuple *b,
+Tuplesortstate *state);
+static int comparetup_heap_1key_asc_nonulls_btint4cmp(const SortTuple *a, const SortTuple *b,
+Tuplesortstate *state);
+static int compare_int4(Datum first, Datum second);
static void copytup_heap(Tuplesortstate *state, SortTuple *stup, void *tup);
static void writetup_heap(Tuplesortstate *state, int tapenum,
SortTuple *stup);
@@ -551,6 +562,7 @@ tuplesort_begin_common(int workMem, bool randomAccess)
state->availMem = state->allowedMem;
state->sortcontext = sortcontext;
state->tapeset = NULL;
+ state->hasnulls_initial = false;
state->memtupcount = 0;
state->memtupsize = 1024; /* initial guess */
@@ -1247,11 +1259,41 @@ tuplesort_performsort(Tuplesortstate *state)
* amount of memory. Just qsort 'em and we're done.
*/
if (state->memtupcount > 1)
+ {
+int (*comparetup) (const SortTuple *a, const SortTuple *b,
+ Tuplesortstate *state);
+
+/* If there is only one key col, the input contains no NULLs,
+ * and it's ascending, we can use a fastpath version of
+ * comparetup_heap that skips the over those things.
+ */
+if (state->comparetup == comparetup_heap &&
+ state->nKeys == 1 &&
+ !(state->scanKeys[0].sk_flags & SK_BT_DESC) &&
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
Kevin, Thank you for your review! On 2011-12-03 21:53, Kevin Grittner wrote: (1) Doc changes: (a) These contain some unnecessary whitespace changes which make it harder to see exactly what changed. This is probably caused because I used emacs's fill-paragraph (alt+q) command, after some changes. If this is against policy, I could change the additions in such a way as to cause minor differences, however I believe that the benefit of that ends immediately after review. (b) There's one point where "cursors" should be possessive "cursor's". (c) I think it would be less confusing to be consistent about mentioning "positional" and "named" in the same order each time. Mixing it up like that is harder to read, at least for me. Good point, both changed. (2) The error for mixing positional and named parameters should be moved up. That seems more important than "too many arguments" or "provided multiple times" if both are true. I moved the error up, though I personally tend to believe it doesn't even need to be an error. There is no technical reason not to allow it. All the user needs to do is make sure that the combination of named parameters and the positional ones together are complete and not overlapping. This is also in line with calling functions, where mixing named and positional is allowed, as long as the positional arguments are first. Personally I have no opinion what is best, include the feature or give an error, and I removed the possibility during the previous commitfest. (3) The "-- END ADDITIONAL TESTS" comment shouldn't be added to the regression tests. This is removed, also the -- START ADDITIONAL TESTS, though I kept the tests between those to comments. The way this parses out the named parameters and then builds the positional list, with some code from read_sql_construct() repeated in read_cursor_args() seems a little bit clumsy to me, but I couldn't think of a better way to do it. Same here. The new patch is attached. regards Yeb Havinga diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml new file mode 100644 index f33cef5..a3e6540 *** a/doc/src/sgml/plpgsql.sgml --- b/doc/src/sgml/plpgsql.sgml *** OPEN curs1 FOR EXECUTE 'SELECT * FROM ' *** 2823,2833 ! Opening a Bound Cursor ! OPEN bound_cursorvar ( argument_values ) ; --- 2823,2833 ! Opening a Bound Cursor ! OPEN bound_cursorvar ( argname := argument_value , ... ) ; *** OPEN bound_cursorvar + Cursors may be opened using either positional + or named notation. In contrast with calling + functions, described in , it + is not allowed to mix positional and named notation. In positional + notation, all arguments are specified in order. In named notation, + each argument's name is specified using := to + separate it from the argument expression. + + + Examples (these use the cursor declaration examples above): OPEN curs2; OPEN curs3(42); + OPEN curs3(key := 42); *** COMMIT; *** 3169,3186 <
Re: [HACKERS] Notes on implementing URI syntax for libpq
On Tue, Nov 29, 2011 at 12:02 PM, Alexander Shulgin
wrote:
>
> Excerpts from Alexander Shulgin's message of Sat Nov 26 22:07:21 +0200 2011:
>>
>> So how about this:
>>
>> postgresql:ssl://user:pw@host:port/dbname?sslmode=...
>>
>> The "postgresql:ssl://" designator would assume "sslmode=require", if not
>> overriden in extra parameters and "postgresql://" would imply
>> "sslmode=prefer". And to disable SSL you would pick either designator and
>> append "sslmode=disable".
>>
>> The JDBC's "ssl=true" will translate to "sslmode=require".
>
> Hey, I'm going to assume "no objections" equals "positive feedback" and
> continue hacking in this direction.
A couple of cents on this:
I think the current direction is fine, although as Robert Haas has
said, I am not really at all inclined to view JDBC compatibility as
any kind of a plus. JDBC URLs are weird, and do the drivers actually
link libpq anyway? That world is unto itself. Looking like a normal
URL to the greater body of URL-dom seems like a much more desirable
design trait to me, and after that decreasing the number of ways to
write the same thing. So a weak -1 from me on adding two ways to do
the same thing, of which the way to do it is weird by URL standards.
At best I'd try to avoid choosing any notations that clash or visually
confuse the URLs with their JDBC doppelgangers, and I think the more
URL-ish notation is polite to JDBC in that regard.
Here's a couple of URIs used by projects that do generally end up
delegating to libpq-based drivers. It is precisely this slight
fragmentation that I'd like to see put to rest by this feature in
libpq.
Sequel, for Ruby (all valid):
DB = Sequel.connect('postgres://user:password@localhost/blog') # Uses
the postgres adapter
DB = Sequel.connect('postgres://localhost/blog?user=user&password=password')
DB = Sequel.connect('postgres://localhost/blog' :user=>'user',
:password=>'password')
Amazingly, I don't find it trivial to find the format ActiveRecord
(part of Rails) spelled out, but here's one thing that works, at least
(same as Sequel, format one)
postgres://username:password@localhost/myrailsdb
Django: Doesn't use URLs at all, preferring dictionary structures, as
far as I can tell.
SQLAlchemy, Python:
dialect+driver://user:password@host/dbname[?key=value..]
I'm not familiar enough with the Perl and TCL worlds to comment, nor
some of the newcomers like Golang or Node.js.
Dialect would be 'postgresql', driver 'psycopg2', but at the libpq
level clearly that wouldn't make much sense, so it's basically Sequel
format one-compatible, except it goes by postgresql rather than
postgres for the dialect.
I do really like the attention paid to somehow making AF_UNIX sockets
work; they're great for development. I agree a different scheme would
be hard to swallow, but unfortunately the options to pack that
information into URL notation is at least a little ugly, as far as I
can tell. Using a different scheme is probably not worth the cost of
an ugly URL string, in my book.
Are there any provisions for choosing X.509/cert authentication? I
imagine not, but out-of-band presentation of that information is the
norm there, and I'm not sure if is any room for improvement within
reach.
>> If we can decide on this, we should also put reasonable effort into making
>> JDBC support the same syntax.
>
> What would be our plan on this? Since the syntax proposed here is strictly a
> superset of the existing JDBC syntax, I would think this qualifies as an
> improvement and it would be backwards compatible with any previous version of
> the JDBC connector.
I suppose that is nice, but is this designed, or coincidental? Is
there any fundamental reason why the JDBC driver will remain so
similar to libpq in the future? Will people realistically be able to
use one URL across their Java and libpq projects in most situations,
now and in the forseeable future, including the keyword options?
Because as soon as one encounters the need to maintain two URLs for
any reason, the otherwise real convenience regresses into bloat.
So there's my pile of opinions.
--
fdr
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Detecting loss of client connection (was Re: Bug in walsender when calling out to do_pg_stop_backup (and others?))
Thinking about this some more, why don't we just elog(FATAL) in
internal_flush(), if the write fails, instead of setting the flag and
waiting for the next CHECK_FOR_INTERRUPTS(). That sounds scary at first,
but why not?
There's this comment in internal_flush():
/*
* Careful: an ereport() that tries to write to the
client would
* cause recursion to here, leading to stack overflow
and core
* dump! This message must go *only* to the postmaster
log.
That's understandable.
* If a client disconnects while we're in the midst of
output, we
* might write quite a bit of data before we get to a
safe query
* abort point. So, suppress duplicate log messages.
But what about this? Tracing back the callers, I don't see any that
would be upset if we just threw an error there. One scary aspect is if
you're within a critical section, but I don't think we currently send
any messages while in a critical section. And we could refrain from
throwing the error if we're in a critical section, to be safe.
*/
if (errno != last_reported_send_errno)
{
last_reported_send_errno = errno;
ereport(COMMERROR,
(errcode_for_socket_access(),
errmsg("could not send data to
client: %m")));
}
--
Heikki Linnakangas
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
