Re: [HACKERS] PL/perl should fail on configure, not make
On 01/08/2013 10:37 PM, Tom Lane wrote: We could try adding an AC_TRY_LINK test using perl_embed_ldflags, but given the weird stuff happening to redefine that value on Windows in plperl/GNUmakefile I think there's a serious risk of breaking Cygwin builds. Since I lack access to either Cygwin or a platform on which there's a problem today, I'm not going to be the one to mess with it. ITYM Mingw - the Makefile doesn't do anything for Cygwin. If you want to build a configure test, you could make it conditional on the PORTNAME not being win32, since we don't seem to have a problem there anyway. 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] Extra XLOG in Checkpoint for StandbySnapshot
On Tuesday, January 08, 2013 8:57 PM Andres Freund wrote:
On 2013-01-08 20:33:28 +0530, Amit Kapila wrote:
On Tuesday, January 08, 2013 8:01 PM Andres Freund wrote:
On 2013-01-08 19:51:39 +0530, Amit Kapila wrote:
On Monday, January 07, 2013 7:15 PM Andres Freund wrote:
On 2013-01-07 19:03:35 +0530, Amit Kapila wrote:
On Monday, January 07, 2013 6:30 PM Simon Riggs wrote:
On 7 January 2013 12:39, Amit Kapila
amit.kap...@huawei.com
wrote:
The information that no transactions are currently running
allows
you
to
build a recovery snapshot, without that information the standby
won't
start answering queries. Now that doesn't matter if all
standbys
already
have built a snapshot, but the primary cannot know that.
Can't we make sure that checkpoint operation doesn't happen for
below
conds.
a. nothing has happened during or after last checkpoint
OR
b. nothing except snapshotstanby WAL has happened
Currently it is done for point a.
Having to issue a checkpoint while ensuring transactions are
running
just to get a standby up doesn't seem like a good idea to me :)
Simon:
If you make the correct test, I'd be more inclined to accept
the
premise.
Not sure, what exact you are expecting from test?
The test is do any one operation on system and then keep the
system
idle.
Now at each checkpoint interval, it logs WAL for SnapshotStandby.
I can't really follow what you want to do here. The snapshot is
only
logged if a checkpoint is performed anyway? As recovery starts at
(the
logical) checkpoint's location we need to log a snapshot exactly
there. If you want to avoid activity when the system is idle you
need
to
prevent checkpoints from occurring itself.
Even if the checkpoint is scheduled, it doesn't perform actual
operation if
there's nothing logged between
current and previous checkpoint due to below check in
CreateCheckPoint()
function.
if (curInsert == ControlFile-checkPoint +
MAXALIGN(SizeOfXLogRecord +
sizeof(CheckPoint))
ControlFile-checkPoint ==
ControlFile-checkPointCopy.redo)
But if we set the wal_level as hot_standby, it will log snapshot, now
next
time again when function CreateCheckPoint()
will get called due to scheduled checkpoint, the above check will
fail and
it will again log snapshot, so this will continue, even if the system
is
totally idle.
I understand that it doesn't cause any problem, but I think it is
better if
the repeated log of snapshot in this scenario can be avoided.
ISTM in that case you just need a way to cope with the additionally
logged record in the above piece of code. Not logging seems to be the
entirely wrong way to go at this.
I think one of the ways code can be modified is as below:
+ /*size of running transactions log when there is no
active transation*/
+if (!shutdown XLogStandbyInfoActive())
+{
+runningXactXLog =
MAXALIGN(MinSizeOfXactRunningXacts) + SizeOfXLogRecord;
+}
!if (curInsert == ControlFile-checkPoint +
!MAXALIGN(SizeOfXLogRecord + sizeof(CheckPoint))
!ControlFile-checkPoint ==
ControlFile-checkPointCopy.redo)
!if (curInsert == ControlFile-checkPoint +
!MAXALIGN(SizeOfXLogRecord + sizeof(CheckPoint))
!ControlFile-checkPoint ==
ControlFile-checkPointCopy.redo + runningXactXLog)
Second condition is checking the last checkpoint WAL position with the
current one.
Since ControlFile-checkPointCopy.redo holds the value before running
Xact WAL was inserted
and ControlFile-checkPoint holds the value after running Xact WAL got
inserted, so if no new WAL was inserted apart from running Xacts and
Checkpoint WAL, then this condition will be true.
Not logging seems to be the entirely wrong way to go at this.
True.
I admit its not totally simple, but making HS less predictable seems
like a cure *far* worse than the disease.
Right, that's why I am trying to figure out if there can be a way to handle
without any compromise on HS.
With Regards,
Amit Kapila.
--
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] proposal: Set effective_cache_size to greater of .conf value, shared_buffers
On Wed, Jan 9, 2013 at 2:01 AM, Josh Berkus j...@agliodbs.com wrote: All, Well, the problem of find out the box's physical RAM is doubtless solvable if we're willing to put enough sweat and tears into it, but I'm dubious that it's worth the trouble. The harder part is how to know if the box is supposed to be dedicated to the database. Bear in mind that the starting point of this debate was the idea that we're talking about an inexperienced DBA who doesn't know about any configuration knob we might provide for the purpose. For what it is worth even if it is a dedicated database box 75% might be way too high. I remember investigating bad performance on our biggest database server, that in the end turned out to be a too high setting of effective_cache_size. From reading the code back then my rationale for it being to high was that the code that makes use of the effective_cache_size tries very hard to account for what the current query would do to the cache but doesn't take into account how many queries (on separate datasets!) are currently begin executed (and competing for the same cache). On that box we often have 100+ active connections and many looking at different big datasets. Cheers, bene
[HACKERS] inconsistent behave of boolean based domains in XML functions
Hello On Czech pg mailing list was reported issue about problems with boolean based domains and XML functions. There are maybe more issues, but probably there is little bit strange and unexpected result postgres=# CREATE DOMAIN booldomain as bool; CREATE DOMAIN -- fully expected behave postgres=# select true, true::booldomain; bool | booldomain --+ t| t (1 row) postgres=# select true::text, true::booldomain::text; text | text --+-- true | true (1 row) -- unexpected behave postgres=# select xmlforest(true as bool, true::booldomain as booldomain); xmlforest - booltrue/boolbooldomaint/booldomain (1 row) Best regards Pavel Stehule -- 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] Extra XLOG in Checkpoint for StandbySnapshot
On 2013-01-09 14:04:32 +0530, Amit Kapila wrote:
On Tuesday, January 08, 2013 8:57 PM Andres Freund wrote:
On 2013-01-08 20:33:28 +0530, Amit Kapila wrote:
On Tuesday, January 08, 2013 8:01 PM Andres Freund wrote:
On 2013-01-08 19:51:39 +0530, Amit Kapila wrote:
On Monday, January 07, 2013 7:15 PM Andres Freund wrote:
On 2013-01-07 19:03:35 +0530, Amit Kapila wrote:
On Monday, January 07, 2013 6:30 PM Simon Riggs wrote:
On 7 January 2013 12:39, Amit Kapila
amit.kap...@huawei.com
wrote:
The information that no transactions are currently running
allows
you
to
build a recovery snapshot, without that information the standby
won't
start answering queries. Now that doesn't matter if all
standbys
already
have built a snapshot, but the primary cannot know that.
Can't we make sure that checkpoint operation doesn't happen for
below
conds.
a. nothing has happened during or after last checkpoint
OR
b. nothing except snapshotstanby WAL has happened
Currently it is done for point a.
Having to issue a checkpoint while ensuring transactions are
running
just to get a standby up doesn't seem like a good idea to me :)
Simon:
If you make the correct test, I'd be more inclined to accept
the
premise.
Not sure, what exact you are expecting from test?
The test is do any one operation on system and then keep the
system
idle.
Now at each checkpoint interval, it logs WAL for SnapshotStandby.
I can't really follow what you want to do here. The snapshot is
only
logged if a checkpoint is performed anyway? As recovery starts at
(the
logical) checkpoint's location we need to log a snapshot exactly
there. If you want to avoid activity when the system is idle you
need
to
prevent checkpoints from occurring itself.
Even if the checkpoint is scheduled, it doesn't perform actual
operation if
there's nothing logged between
current and previous checkpoint due to below check in
CreateCheckPoint()
function.
if (curInsert == ControlFile-checkPoint +
MAXALIGN(SizeOfXLogRecord +
sizeof(CheckPoint))
ControlFile-checkPoint ==
ControlFile-checkPointCopy.redo)
But if we set the wal_level as hot_standby, it will log snapshot, now
next
time again when function CreateCheckPoint()
will get called due to scheduled checkpoint, the above check will
fail and
it will again log snapshot, so this will continue, even if the system
is
totally idle.
I understand that it doesn't cause any problem, but I think it is
better if
the repeated log of snapshot in this scenario can be avoided.
ISTM in that case you just need a way to cope with the additionally
logged record in the above piece of code. Not logging seems to be the
entirely wrong way to go at this.
I think one of the ways code can be modified is as below:
+ /*size of running transactions log when there is no
active transation*/
+if (!shutdown XLogStandbyInfoActive())
+{
+runningXactXLog =
MAXALIGN(MinSizeOfXactRunningXacts) + SizeOfXLogRecord;
+}
!if (curInsert == ControlFile-checkPoint +
!MAXALIGN(SizeOfXLogRecord + sizeof(CheckPoint))
!ControlFile-checkPoint ==
ControlFile-checkPointCopy.redo)
!if (curInsert == ControlFile-checkPoint +
!MAXALIGN(SizeOfXLogRecord + sizeof(CheckPoint))
!ControlFile-checkPoint ==
ControlFile-checkPointCopy.redo + runningXactXLog)
Second condition is checking the last checkpoint WAL position with the
current one.
Since ControlFile-checkPointCopy.redo holds the value before running
Xact WAL was inserted
and ControlFile-checkPoint holds the value after running Xact WAL got
inserted, so if no new WAL was inserted apart from running Xacts and
Checkpoint WAL, then this condition will be true.
I don't think thats safe, there could have been another record inserted
that happens to be MinSizeOfXactRunningXacts big and we would still skip
the checkpoint.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Cascading replication: should we detect/prevent cycles?
On 9 January 2013 01:51, Josh Berkus j...@agliodbs.com wrote: Anyway, I'm not saying we solve this now. I'm saying, put it on the TODO list in case someone has time/an itch to scratch. I think its reasonable to ask whether a usability feature needs to exist whenever a problem is encountered. That shouldn't need to translate to a new feature/TODO every time we ask the question though. IMHO, in this case, we should document this as an issue that can happen and we should caution that careful testing is required. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extra XLOG in Checkpoint for StandbySnapshot
On Wednesday, January 09, 2013 2:28 PM Andres Freund wrote:
On 2013-01-09 14:04:32 +0530, Amit Kapila wrote:
On Tuesday, January 08, 2013 8:57 PM Andres Freund wrote:
On 2013-01-08 20:33:28 +0530, Amit Kapila wrote:
On Tuesday, January 08, 2013 8:01 PM Andres Freund wrote:
On 2013-01-08 19:51:39 +0530, Amit Kapila wrote:
On Monday, January 07, 2013 7:15 PM Andres Freund wrote:
On 2013-01-07 19:03:35 +0530, Amit Kapila wrote:
On Monday, January 07, 2013 6:30 PM Simon Riggs wrote:
On 7 January 2013 12:39, Amit Kapila
amit.kap...@huawei.com
wrote:
The information that no transactions are currently running
allows
you
to
build a recovery snapshot, without that information the
standby
won't
start answering queries. Now that doesn't matter if all
standbys
already
have built a snapshot, but the primary cannot know that.
Can't we make sure that checkpoint operation doesn't happen
for
below
conds.
a. nothing has happened during or after last checkpoint
OR
b. nothing except snapshotstanby WAL has happened
Currently it is done for point a.
Having to issue a checkpoint while ensuring transactions
are
running
just to get a standby up doesn't seem like a good idea to
me :)
Simon:
If you make the correct test, I'd be more inclined to
accept
the
premise.
Not sure, what exact you are expecting from test?
The test is do any one operation on system and then keep the
system
idle.
Now at each checkpoint interval, it logs WAL for
SnapshotStandby.
I can't really follow what you want to do here. The snapshot is
only
logged if a checkpoint is performed anyway? As recovery starts
at
(the
logical) checkpoint's location we need to log a snapshot
exactly
there. If you want to avoid activity when the system is idle
you
need
to
prevent checkpoints from occurring itself.
Even if the checkpoint is scheduled, it doesn't perform actual
operation if
there's nothing logged between
current and previous checkpoint due to below check in
CreateCheckPoint()
function.
if (curInsert == ControlFile-checkPoint +
MAXALIGN(SizeOfXLogRecord +
sizeof(CheckPoint))
ControlFile-checkPoint ==
ControlFile-checkPointCopy.redo)
But if we set the wal_level as hot_standby, it will log snapshot,
now
next
time again when function CreateCheckPoint()
will get called due to scheduled checkpoint, the above check will
fail and
it will again log snapshot, so this will continue, even if the
system
is
totally idle.
I understand that it doesn't cause any problem, but I think it is
better if
the repeated log of snapshot in this scenario can be avoided.
ISTM in that case you just need a way to cope with the
additionally
logged record in the above piece of code. Not logging seems to be
the
entirely wrong way to go at this.
I think one of the ways code can be modified is as below:
+ /*size of running transactions log when there is no
active transation*/
+if (!shutdown XLogStandbyInfoActive())
+{
+runningXactXLog =
MAXALIGN(MinSizeOfXactRunningXacts) + SizeOfXLogRecord;
+}
!if (curInsert == ControlFile-checkPoint +
!MAXALIGN(SizeOfXLogRecord +
sizeof(CheckPoint))
!ControlFile-checkPoint ==
ControlFile-checkPointCopy.redo)
!if (curInsert == ControlFile-checkPoint +
!MAXALIGN(SizeOfXLogRecord +
sizeof(CheckPoint))
!ControlFile-checkPoint ==
ControlFile-checkPointCopy.redo + runningXactXLog)
Second condition is checking the last checkpoint WAL position with
the
current one.
Since ControlFile-checkPointCopy.redo holds the value before
running
Xact WAL was inserted
and ControlFile-checkPoint holds the value after running Xact WAL
got
inserted, so if no new WAL was inserted apart from running Xacts
and
Checkpoint WAL, then this condition will be true.
I don't think thats safe, there could have been another record inserted
that happens to be MinSizeOfXactRunningXacts big and we would still
skip the checkpoint.
I think such can happen only for when first time checkpoint is triggered,
and even then the first part of the check (curInsert ==
ControlFile-checkPoint + MAXALIGN(SizeOfXLogRecord + sizeof(CheckPoint))
will fail.
Value to runningXactXLog will be assigned only if wal_level is hot_stanby.
In that case if checkpoint is getting scheduled for 2nd or consecutive time,
it will include WAL for running Xact along with WAL for any other data.
Re: [HACKERS] PL/perl should fail on configure, not make
Re: Tom Lane 2013-01-09 9802.1357702...@sss.pgh.pa.us Item: there is not a test for perl.h, as such, in configure. There probably should be, just because we have comparable tests for tcl.h and Python.h. However, adding one won't fix your problem on Debian-based distros, because for some wacko reason they put the headers and the shlib .so symlink in different packages, cf http://packages.debian.org/squeeze/amd64/perl/filelist http://packages.debian.org/squeeze/amd64/libperl-dev/filelist I am unfamiliar with a good reason for doing that. (I can certainly see segregating the .a static library, or even not shipping it at all, but what's it save to leave out the .so symlink?) Because the .so symlink is only needed at build time. At runtime, you need the .so.5.14 file. Hence .so.* - $pkg, .h .a .so - $pkg-dev. Christoph -- c...@df7cb.de | http://www.df7cb.de/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Proposal: Store timestamptz of database creation on pg_database
One thing i'd really like to be in this common object info catalog is DDL which created or altered the referenced object. If we additionally could make it possible to have ordinary triggers on this catalog it would solve most logical DDL replication problems Hannu Sent from Samsung Galaxy NotePeter Eisentraut pete...@gmx.net wrote:On Tue, 2013-01-08 at 17:17 -0500, Stephen Frost wrote: Seriously tho, the argument for not putting these things into the various individual catalogs is that they'd create bloat and these items don't need to be performant. I would think that the kind of timestamps that we're talking about fall into the same data category as comments on tables. If there isn't a good reason for comments on objects to be off in a generic this is for any kind of object table, then perhaps we should move them into the appropriate catalog tables? I think basic refactoring logic would support taking common things out of the individual catalogs and keeping them in a common structure, especially when they are for amusement only and not needed in any critical paths. All the ALTER command refactoring and so on that's been going on is also moving into the direction that for data definition management, there should be mainly one kind of object with a few variants here and there.
Re: [HACKERS] Re: patch submission: truncate trailing nulls from heap rows to reduce the size of the null bitmap [Review]
On 24 December 2012 16:57, Amit Kapila amit.kap...@huawei.com wrote: Performance: Average of 3 runs of pgbench in tps 9.3devel | with trailing null patch --+-- 578.9872 | 573.4980 On balance, it would seem optimizing for this special case would affect everybody negatively; not much, but enough. Which means we should rekect this patch. Do you have a reason why a different view might be taken? -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Performance Improvement by reducing WAL for Update Operation
On 9 January 2013 08:05, Amit kapila amit.kap...@huawei.com wrote: Update patch contains handling of below Comments Thanks Test results with modified pgbench (1800 record size) on the latest patch: -Patch- -tps@-c1- -WAL@-c1- -tps@-c2- -WAL@-c2- Head831 4.17 GB1416 7.13 GB WAL modification846 2.36 GB1712 3.31 GB -Patch- -tps@-c4- -WAL@-c4- -tps@-c8- -WAL@-c8- Head2196 11.01 GB 2758 13.88 GB WAL modification3295 5.87 GB 54729.02 GB And test results on normal pgbench? -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH 2/2] use pg_malloc instead of an unchecked malloc in pg_resetxlog
--- src/bin/pg_resetxlog/pg_resetxlog.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/bin/pg_resetxlog/pg_resetxlog.c b/src/bin/pg_resetxlog/pg_resetxlog.c index 8734f2c..60fb30c 100644 --- a/src/bin/pg_resetxlog/pg_resetxlog.c +++ b/src/bin/pg_resetxlog/pg_resetxlog.c @@ -54,6 +54,7 @@ #include access/xlog_internal.h #include catalog/catversion.h #include catalog/pg_control.h +#include port/palloc.h extern int optind; extern char *optarg; @@ -390,7 +391,7 @@ ReadControlFile(void) } /* Use malloc to ensure we have a maxaligned buffer */ - buffer = (char *) malloc(PG_CONTROL_SIZE); + buffer = (char *) pg_malloc(PG_CONTROL_SIZE); len = read(fd, buffer, PG_CONTROL_SIZE); if (len 0) @@ -904,7 +905,7 @@ WriteEmptyXLOG(void) int nbytes; /* Use malloc() to ensure buffer is MAXALIGNED */ - buffer = (char *) malloc(XLOG_BLCKSZ); + buffer = (char *) pg_malloc(XLOG_BLCKSZ); page = (XLogPageHeader) buffer; memset(buffer, 0, XLOG_BLCKSZ); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Hi, As promised here's a patch to provide palloc emulation for frontend-ish environments. The patch: - makes palloc() into a real function so CurrentMemoryContext doesn't need to be provided - provides common pg_(malloc,malloc0, realloc, strdup, free) wrappers and removes various versions of those across different utilities - removes ugly palloc redefinery for frontend use of backend code (dirmod.c) Controversial/Unclear things: - palloc[0] are currently copies of the MemoryContextAlloc[Zero] functions to preclude performance regressions, imo the level of duplication is ok though - the common memory management is implemented in [pg]port/palloc.[ch], I am not too happy with the name and location - pgport/palloc.c is only built in the backend, not sure if there is a nicer way to do this from a make POV - the different versions of pg_malloc et al used different error signaling methods, I've settled on fprintf(stderr, _(out of memory\n)); exit(EXIT_FAILURE); Results in a nice net removal of code: 37 files changed, 218 insertions(+), 621 deletions(-) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] information schema parameter_default implementation
Here is an implementation of the
information_schema.parameters.parameter_default column.
I ended up writing a C function to decode the whole thing from the
system catalogs, because it was too complicated in SQL, so I abandoned
the approach discussed in [0].
[0]:
http://archives.postgresql.org/message-id/1356092400.25658.6.ca...@vanquo.pezone.net
diff --git a/doc/src/sgml/information_schema.sgml b/doc/src/sgml/information_schema.sgml
index ddbc56c..4fa4ab8 100644
--- a/doc/src/sgml/information_schema.sgml
+++ b/doc/src/sgml/information_schema.sgml
@@ -3323,6 +3323,15 @@ titleliteralparameters/literal Columns/title
in future versions.)
/entry
/row
+
+ row
+ entryliteralparameter_default/literal/entry
+ entrytypecharacter_data/type/entry
+ entry
+ The default expression of the parameter, or null if none or if the
+ function is not owned by a currently enabled role.
+ /entry
+ /row
/tbody
/tgroup
/table
diff --git a/src/backend/catalog/information_schema.sql b/src/backend/catalog/information_schema.sql
index 2307586..82d686a 100644
--- a/src/backend/catalog/information_schema.sql
+++ b/src/backend/catalog/information_schema.sql
@@ -1132,10 +1132,15 @@ CREATE VIEW parameters AS
CAST(null AS sql_identifier) AS scope_schema,
CAST(null AS sql_identifier) AS scope_name,
CAST(null AS cardinal_number) AS maximum_cardinality,
- CAST((ss.x).n AS sql_identifier) AS dtd_identifier
+ CAST((ss.x).n AS sql_identifier) AS dtd_identifier,
+ CAST(
+ CASE WHEN pg_has_role(proowner, 'USAGE')
+ THEN pg_get_function_arg_default(p_oid, (ss.x).n)
+ ELSE NULL END
+ AS character_data) AS parameter_default
FROM pg_type t, pg_namespace nt,
- (SELECT n.nspname AS n_nspname, p.proname, p.oid AS p_oid,
+ (SELECT n.nspname AS n_nspname, p.proname, p.oid AS p_oid, p.proowner,
p.proargnames, p.proargmodes,
_pg_expandarray(coalesce(p.proallargtypes, p.proargtypes::oid[])) AS x
FROM pg_namespace n, pg_proc p
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 266cec5..b9ebb78 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -2248,6 +2248,76 @@ static char *generate_function_name(Oid funcid, int nargs, List *argnames,
return argsprinted;
}
+Datum
+pg_get_function_arg_default(PG_FUNCTION_ARGS)
+{
+ Oid funcid = PG_GETARG_OID(0);
+ int32 argn = PG_GETARG_INT32(1);
+ HeapTuple proctup;
+ Form_pg_proc proc;
+ int numargs;
+ Oid *argtypes;
+ char **argnames;
+ char *argmodes;
+ int i;
+ List *argdefaults;
+ Node *node;
+ char *str;
+ int inputargn;
+ Datum proargdefaults;
+ bool isnull;
+ int nth;
+
+ proctup = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcid));
+ if (!HeapTupleIsValid(proctup))
+ elog(ERROR, cache lookup failed for function %u, funcid);
+
+ numargs = get_func_arg_info(proctup, argtypes, argnames, argmodes);
+ if (argn numargs)
+ {
+ ReleaseSysCache(proctup);
+ PG_RETURN_NULL();
+ }
+
+ inputargn = 0;
+
+ for (i = 0; i argn; i++)
+ {
+ if (!argmodes || argmodes[i] == PROARGMODE_IN || argmodes[i] == PROARGMODE_INOUT || argmodes[i] == PROARGMODE_VARIADIC)
+ inputargn++;
+ }
+
+ proargdefaults = SysCacheGetAttr(PROCOID, proctup,
+ Anum_pg_proc_proargdefaults,
+ isnull);
+
+ if (isnull)
+ {
+ ReleaseSysCache(proctup);
+ PG_RETURN_NULL();
+ }
+
+ str = TextDatumGetCString(proargdefaults);
+ argdefaults = (List *) stringToNode(str);
+ Assert(IsA(argdefaults, List));
+ pfree(str);
+
+ proc = (Form_pg_proc) GETSTRUCT(proctup);
+
+ nth = inputargn - 1 - (proc-pronargs - proc-pronargdefaults);
+ if (nth 0 || nth = list_length(argdefaults))
+ {
+ ReleaseSysCache(proctup);
+ PG_RETURN_NULL();
+ }
+ node = list_nth(argdefaults, nth);
+ str = deparse_expression_pretty(node, NIL, false, false, 0, 0);
+
+ ReleaseSysCache(proctup);
+
+ PG_RETURN_TEXT_P(string_to_text(str));
+}
+
/*
* deparse_expression - General utility for deparsing expressions
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 1e235c6..dc38532 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
*/
/* mmddN */
-#define CATALOG_VERSION_NO 201212081
+#define CATALOG_VERSION_NO 201212261
#endif
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 010605d..64fbe7e 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -1964,6 +1964,8 @@ DATA(insert OID = 2232 ( pg_get_function_identity_arguments PGNSP PGUID 12 1
DESCR(identity argument list of a function);
DATA(insert OID = 2165 ( pg_get_function_result PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 25 26 _null_ _null_ _null_ _null_ pg_get_function_result
[HACKERS] [PATCH 1/2] Provide a common malloc wrappers and palloc et al. emulation for frontend'ish environs
---
contrib/oid2name/oid2name.c| 52 +
contrib/pg_upgrade/pg_upgrade.h| 5 +-
contrib/pg_upgrade/util.c | 49 -
contrib/pgbench/pgbench.c | 54 +-
src/backend/utils/mmgr/mcxt.c | 78 +++-
src/bin/initdb/initdb.c| 40 +-
src/bin/pg_basebackup/pg_basebackup.c | 2 +-
src/bin/pg_basebackup/pg_receivexlog.c | 1 +
src/bin/pg_basebackup/receivelog.c | 1 +
src/bin/pg_basebackup/streamutil.c | 38 +-
src/bin/pg_basebackup/streamutil.h | 4 -
src/bin/pg_ctl/pg_ctl.c| 39 +-
src/bin/pg_dump/Makefile | 6 +-
src/bin/pg_dump/common.c | 1 -
src/bin/pg_dump/compress_io.c | 1 -
src/bin/pg_dump/dumpmem.c | 76 ---
src/bin/pg_dump/dumpmem.h | 22 --
src/bin/pg_dump/dumputils.h| 1 +
src/bin/pg_dump/pg_backup_archiver.c | 1 -
src/bin/pg_dump/pg_backup_custom.c | 2 +-
src/bin/pg_dump/pg_backup_db.c | 1 -
src/bin/pg_dump/pg_backup_directory.c | 1 -
src/bin/pg_dump/pg_backup_null.c | 1 -
src/bin/pg_dump/pg_backup_tar.c| 1 -
src/bin/pg_dump/pg_dump.c | 1 -
src/bin/pg_dump/pg_dump_sort.c | 1 -
src/bin/pg_dump/pg_dumpall.c | 1 -
src/bin/pg_dump/pg_restore.c | 1 -
src/bin/psql/common.c | 50 -
src/bin/psql/common.h | 10 +--
src/bin/scripts/common.c | 49 -
src/bin/scripts/common.h | 5 +-
src/include/port/palloc.h | 19 +
src/include/utils/palloc.h | 12 +--
src/port/Makefile | 8 +-
src/port/dirmod.c | 75 +--
src/port/palloc.c | 130 +
37 files changed, 218 insertions(+), 621 deletions(-)
delete mode 100644 src/bin/pg_dump/dumpmem.c
delete mode 100644 src/bin/pg_dump/dumpmem.h
create mode 100644 src/include/port/palloc.h
create mode 100644 src/port/palloc.c
diff --git a/contrib/oid2name/oid2name.c b/contrib/oid2name/oid2name.c
index a666731..dfd8105 100644
--- a/contrib/oid2name/oid2name.c
+++ b/contrib/oid2name/oid2name.c
@@ -9,6 +9,8 @@
*/
#include postgres_fe.h
+#include port/palloc.h
+
#include unistd.h
#ifdef HAVE_GETOPT_H
#include getopt.h
@@ -50,9 +52,6 @@ struct options
/* function prototypes */
static void help(const char *progname);
void get_opts(int, char **, struct options *);
-void *pg_malloc(size_t size);
-void *pg_realloc(void *ptr, size_t size);
-char *pg_strdup(const char *str);
void add_one_elt(char *eltname, eary *eary);
char *get_comma_elts(eary *eary);
PGconn *sql_conn(struct options *);
@@ -201,53 +200,6 @@ help(const char *progname)
progname, progname);
}
-void *
-pg_malloc(size_t size)
-{
- void *ptr;
-
- /* Avoid unportable behavior of malloc(0) */
- if (size == 0)
- size = 1;
- ptr = malloc(size);
- if (!ptr)
- {
- fprintf(stderr, out of memory\n);
- exit(1);
- }
- return ptr;
-}
-
-void *
-pg_realloc(void *ptr, size_t size)
-{
- void *result;
-
- /* Avoid unportable behavior of realloc(NULL, 0) */
- if (ptr == NULL size == 0)
- size = 1;
- result = realloc(ptr, size);
- if (!result)
- {
- fprintf(stderr, out of memory\n);
- exit(1);
- }
- return result;
-}
-
-char *
-pg_strdup(const char *str)
-{
- char *result = strdup(str);
-
- if (!result)
- {
- fprintf(stderr, out of memory\n);
- exit(1);
- }
- return result;
-}
-
/*
* add_one_elt
*
diff --git a/contrib/pg_upgrade/pg_upgrade.h b/contrib/pg_upgrade/pg_upgrade.h
index c1a2f53..3324918 100644
--- a/contrib/pg_upgrade/pg_upgrade.h
+++ b/contrib/pg_upgrade/pg_upgrade.h
@@ -11,6 +11,7 @@
#include sys/time.h
#include libpq-fe.h
+#include port/palloc.h
/* Use port in the private/dynamic port number range */
#define DEF_PGUPORT 50432
@@ -438,10 +439,6 @@ void
prep_status(const char *fmt,...)
__attribute__((format(PG_PRINTF_ATTRIBUTE, 1, 2)));
void check_ok(void);
-char *pg_strdup(const char *s);
-void *pg_malloc(size_t size);
-void *pg_realloc(void *ptr, size_t size);
-void pg_free(void *ptr);
const char *getErrorText(int errNum);
unsigned int str2uint(const char *str);
void pg_putenv(const char *var, const char *val);
diff --git a/contrib/pg_upgrade/util.c b/contrib/pg_upgrade/util.c
index c91003a..80d0733 100644
--- a/contrib/pg_upgrade/util.c
+++ b/contrib/pg_upgrade/util.c
@@ -213,55 +213,6 @@ get_user_info(char **user_name)
}
-void *
-pg_malloc(size_t size)
-{
- void *p;
-
- /* Avoid unportable behavior of malloc(0) */
- if (size == 0)
- size = 1;
- p = malloc(size);
- if (p == NULL)
- pg_log(PG_FATAL, %s: out of memory\n, os_info.progname);
- return p;
-}
-
-void *
-pg_realloc(void *ptr, size_t size)
-{
- void *p;
-
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
On 09.01.2013 13:27, Andres Freund wrote: - makes palloc() into a real function so CurrentMemoryContext doesn't need to be provided I don't understand the need for this change. Can't you just: #define palloc(s) pg_malloc(s) in the frontend context? - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further pg_upgrade analysis for many tables
On 23 November 2012 22:34, Jeff Janes jeff.ja...@gmail.com wrote: I got rid of need_eoxact_work entirely and replaced it with a short list that fulfills the functions of indicating that work is needed, and suggesting which rels might need that work. There is no attempt to prevent duplicates, nor to remove invalidated entries from the list. Invalid entries are skipped when the hash entry is not found, and processing is idempotent so duplicates are not a problem. Formally speaking, if MAX_EOXACT_LIST were 0, so that the list overflowed the first time it was accessed, then it would be identical to the current behavior or having only a flag. So formally all I did was increase the max from 0 to 10. ... It is not obvious what value to set the MAX list size to. A few questions, that may help you... Why did you pick 10, when your create temp table example needs 110? Why does the list not grow as needed? -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
On 2013-01-09 13:46:53 +0200, Heikki Linnakangas wrote: On 09.01.2013 13:27, Andres Freund wrote: - makes palloc() into a real function so CurrentMemoryContext doesn't need to be provided I don't understand the need for this change. Can't you just: #define palloc(s) pg_malloc(s) in the frontend context? Yes, that would be possible, but imo its the inferior solution: * it precludes ever sharing code without compiling twice * removing allows us to get rid of the following ugliness in dirmod.c: -#ifndef FRONTEND - -/* - * On Windows, call non-macro versions of palloc; we can't reference - * CurrentMemoryContext in this file because of PGDLLIMPORT conflict. - */ -#if defined(WIN32) || defined(__CYGWIN__) -#undef palloc -#undef pstrdup -#define palloc(sz) pgport_palloc(sz) -#define pstrdup(str) pgport_pstrdup(str) -#endif -#else /* FRONTEND */ - * it opens the window for moving more stuff from utils/palloc.h to memutils.h Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Further pg_upgrade analysis for many tables
On 9 November 2012 18:50, Jeff Janes jeff.ja...@gmail.com wrote: quadratic behavior in the resource owner/lock table I didn't want to let that particular phrase go by without saying exactly what behaviour is that?, so we can discuss fixing that also. This maybe something I already know about, but its worth asking about. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: patch submission: truncate trailing nulls from heap rows to reduce the size of the null bitmap [Review]
On Wednesday, January 09, 2013 4:52 PM Simon Riggs wrote: On 24 December 2012 16:57, Amit Kapila amit.kap...@huawei.com wrote: Performance: Average of 3 runs of pgbench in tps 9.3devel | with trailing null patch --+-- 578.9872 | 573.4980 On balance, it would seem optimizing for this special case would affect everybody negatively; not much, but enough. Which means we should rekect this patch. Do you have a reason why a different view might be taken? I have tried to dig why this gap is coming. I have observed that there is very less change in normal path. I wanted to give it some more time to exactly find if something can be done to avoid performance dip in normal execution. Right now I am busy in certain other work. But definitely in coming week or so, I shall spare time to work on it again. With Regards, Amit Kapila. -- 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] Performance Improvement by reducing WAL for Update Operation
On Wednesday, January 09, 2013 4:57 PM Simon Riggs wrote: On 9 January 2013 08:05, Amit kapila amit.kap...@huawei.com wrote: Update patch contains handling of below Comments Thanks Test results with modified pgbench (1800 record size) on the latest patch: -Patch- -tps@-c1- -WAL@-c1- -tps@-c2- - WAL@-c2- Head831 4.17 GB1416 7.13 GB WAL modification846 2.36 GB1712 3.31 GB -Patch- -tps@-c4- -WAL@-c4- -tps@-c8- - WAL@-c8- Head2196 11.01 GB 2758 13.88 GB WAL modification3295 5.87 GB 54729.02 GB And test results on normal pgbench? As there was no gain for original pgbench as was shown in performance readings, so I thought it is not mandatory. However I shall run for normal pgbench as it should not lead any further dip in normal pgbench. Thanks for pointing. With Regards, Amit Kapila. -- 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 submission: truncate trailing nulls from heap rows to reduce the size of the null bitmap [Review]
On 9 January 2013 12:06, Amit Kapila amit.kap...@huawei.com wrote: On Wednesday, January 09, 2013 4:52 PM Simon Riggs wrote: On 24 December 2012 16:57, Amit Kapila amit.kap...@huawei.com wrote: Performance: Average of 3 runs of pgbench in tps 9.3devel | with trailing null patch --+-- 578.9872 | 573.4980 On balance, it would seem optimizing for this special case would affect everybody negatively; not much, but enough. Which means we should rekect this patch. Do you have a reason why a different view might be taken? I have tried to dig why this gap is coming. I have observed that there is very less change in normal path. I wanted to give it some more time to exactly find if something can be done to avoid performance dip in normal execution. Right now I am busy in certain other work. But definitely in coming week or so, I shall spare time to work on it again. Perhaps. Not every idea produces useful outcomes. Even after your excellent research, it appears we haven't made this work yet. It's a shame. Should we invest more time? It's considered rude to advise others how to spend their time, but let me say this: we simply don't have enough time to do everything and we need to be selective, prioritising our time on to the things that look to give the best benefit. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extra XLOG in Checkpoint for StandbySnapshot
On 2013-01-09 15:06:04 +0530, Amit Kapila wrote:
On Wednesday, January 09, 2013 2:28 PM Andres Freund wrote:
On 2013-01-09 14:04:32 +0530, Amit Kapila wrote:
On Tuesday, January 08, 2013 8:57 PM Andres Freund wrote:
On 2013-01-08 20:33:28 +0530, Amit Kapila wrote:
On Tuesday, January 08, 2013 8:01 PM Andres Freund wrote:
On 2013-01-08 19:51:39 +0530, Amit Kapila wrote:
On Monday, January 07, 2013 7:15 PM Andres Freund wrote:
On 2013-01-07 19:03:35 +0530, Amit Kapila wrote:
On Monday, January 07, 2013 6:30 PM Simon Riggs wrote:
On 7 January 2013 12:39, Amit Kapila
amit.kap...@huawei.com
wrote:
The information that no transactions are currently running
allows
you
to
build a recovery snapshot, without that information the
standby
won't
start answering queries. Now that doesn't matter if all
standbys
already
have built a snapshot, but the primary cannot know that.
Can't we make sure that checkpoint operation doesn't happen
for
below
conds.
a. nothing has happened during or after last checkpoint
OR
b. nothing except snapshotstanby WAL has happened
Currently it is done for point a.
Having to issue a checkpoint while ensuring transactions
are
running
just to get a standby up doesn't seem like a good idea to
me :)
Simon:
If you make the correct test, I'd be more inclined to
accept
the
premise.
Not sure, what exact you are expecting from test?
The test is do any one operation on system and then keep the
system
idle.
Now at each checkpoint interval, it logs WAL for
SnapshotStandby.
I can't really follow what you want to do here. The snapshot is
only
logged if a checkpoint is performed anyway? As recovery starts
at
(the
logical) checkpoint's location we need to log a snapshot
exactly
there. If you want to avoid activity when the system is idle
you
need
to
prevent checkpoints from occurring itself.
Even if the checkpoint is scheduled, it doesn't perform actual
operation if
there's nothing logged between
current and previous checkpoint due to below check in
CreateCheckPoint()
function.
if (curInsert == ControlFile-checkPoint +
MAXALIGN(SizeOfXLogRecord +
sizeof(CheckPoint))
ControlFile-checkPoint ==
ControlFile-checkPointCopy.redo)
But if we set the wal_level as hot_standby, it will log snapshot,
now
next
time again when function CreateCheckPoint()
will get called due to scheduled checkpoint, the above check will
fail and
it will again log snapshot, so this will continue, even if the
system
is
totally idle.
I understand that it doesn't cause any problem, but I think it is
better if
the repeated log of snapshot in this scenario can be avoided.
ISTM in that case you just need a way to cope with the
additionally
logged record in the above piece of code. Not logging seems to be
the
entirely wrong way to go at this.
I think one of the ways code can be modified is as below:
+ /*size of running transactions log when there is no
active transation*/
+if (!shutdown XLogStandbyInfoActive())
+{
+runningXactXLog =
MAXALIGN(MinSizeOfXactRunningXacts) + SizeOfXLogRecord;
+}
!if (curInsert == ControlFile-checkPoint +
!MAXALIGN(SizeOfXLogRecord +
sizeof(CheckPoint))
!ControlFile-checkPoint ==
ControlFile-checkPointCopy.redo)
!if (curInsert == ControlFile-checkPoint +
!MAXALIGN(SizeOfXLogRecord +
sizeof(CheckPoint))
!ControlFile-checkPoint ==
ControlFile-checkPointCopy.redo + runningXactXLog)
Second condition is checking the last checkpoint WAL position with
the
current one.
Since ControlFile-checkPointCopy.redo holds the value before
running
Xact WAL was inserted
and ControlFile-checkPoint holds the value after running Xact WAL
got
inserted, so if no new WAL was inserted apart from running Xacts
and
Checkpoint WAL, then this condition will be true.
I don't think thats safe, there could have been another record inserted
that happens to be MinSizeOfXactRunningXacts big and we would still
skip the checkpoint.
I think such can happen only for when first time checkpoint is triggered,
and even then the first part of the check (curInsert ==
ControlFile-checkPoint + MAXALIGN(SizeOfXLogRecord + sizeof(CheckPoint))
will fail.
Value to runningXactXLog will be
Re: [HACKERS] [PATCH 1/2] Provide a common malloc wrappers and palloc et al. emulation for frontend'ish environs
Am I the only one who finds this way of posting patches really annoying? Here is a patch with no description other than a list of changed files. And discussion happens in a completely different email. What's wrong with just posting the patch as a regular attachment(s) to a regular thread, like other people do? Yes, I'm well aware that some mailers thread them in right. Our archives code does. But many MUAs don't. But even when using a MUA that threads it correctly, I find it quite annoying that you have to open one email to read the comments and a different email toview the patch. It may be just me. But it may be others as well, so I figured I should raise the issue :) //Magnus On Wed, Jan 9, 2013 at 12:27 PM, Andres Freund and...@2ndquadrant.com wrote: --- contrib/oid2name/oid2name.c| 52 + contrib/pg_upgrade/pg_upgrade.h| 5 +- contrib/pg_upgrade/util.c | 49 - contrib/pgbench/pgbench.c | 54 +- src/backend/utils/mmgr/mcxt.c | 78 +++- src/bin/initdb/initdb.c| 40 +- src/bin/pg_basebackup/pg_basebackup.c | 2 +- src/bin/pg_basebackup/pg_receivexlog.c | 1 + src/bin/pg_basebackup/receivelog.c | 1 + src/bin/pg_basebackup/streamutil.c | 38 +- src/bin/pg_basebackup/streamutil.h | 4 - src/bin/pg_ctl/pg_ctl.c| 39 +- src/bin/pg_dump/Makefile | 6 +- src/bin/pg_dump/common.c | 1 - src/bin/pg_dump/compress_io.c | 1 - src/bin/pg_dump/dumpmem.c | 76 --- src/bin/pg_dump/dumpmem.h | 22 -- src/bin/pg_dump/dumputils.h| 1 + src/bin/pg_dump/pg_backup_archiver.c | 1 - src/bin/pg_dump/pg_backup_custom.c | 2 +- src/bin/pg_dump/pg_backup_db.c | 1 - src/bin/pg_dump/pg_backup_directory.c | 1 - src/bin/pg_dump/pg_backup_null.c | 1 - src/bin/pg_dump/pg_backup_tar.c| 1 - src/bin/pg_dump/pg_dump.c | 1 - src/bin/pg_dump/pg_dump_sort.c | 1 - src/bin/pg_dump/pg_dumpall.c | 1 - src/bin/pg_dump/pg_restore.c | 1 - src/bin/psql/common.c | 50 - src/bin/psql/common.h | 10 +-- src/bin/scripts/common.c | 49 - src/bin/scripts/common.h | 5 +- src/include/port/palloc.h | 19 + src/include/utils/palloc.h | 12 +-- src/port/Makefile | 8 +- src/port/dirmod.c | 75 +-- src/port/palloc.c | 130 + 37 files changed, 218 insertions(+), 621 deletions(-) delete mode 100644 src/bin/pg_dump/dumpmem.c delete mode 100644 src/bin/pg_dump/dumpmem.h create mode 100644 src/include/port/palloc.h create mode 100644 src/port/palloc.c -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Commitfest Topics
I've set up commifest topics for CFJan15. This will allow people to move across any patches from earlier commitfests. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH 1/2] Provide a common malloc wrappers and palloc et al. emulation for frontend'ish environs
On 2013-01-09 13:34:12 +0100, Magnus Hagander wrote: Am I the only one who finds this way of posting patches really annoying? Well, I unsurprisingly don't ;) Here is a patch with no description other than a list of changed files. And discussion happens in a completely different email. They contain the commit message - which in most of the cases is more informative than the one just posted, which was definitely rather short. It should like in e.g. http://archives.postgresql.org/message-id/1352942234-3953-11-git-send-email-andres%402ndquadrant.com What's wrong with just posting the patch as a regular attachment(s) to a regular thread, like other people do? Two issues: - If you have a bigger series of patches (like the whole logical decoding thing) posting all patches in a single mail makes the following thread even harder to follow than its currently the case. Note how even in this, far smaller, case the discussion actually happened in the appropriate subthreads. I find it way much easier to reread through an old thread that way to reassure myself what was discussed. - mhonarc does really strange things if you attach two git created patches (splits them into multiple mails) It may be just me. But it may be others as well, so I figured I should raise the issue :) I am happy to comply with whatever others prefer. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH 1/2] Provide a common malloc wrappers and palloc et al. emulation for frontend'ish environs
On Wed, Jan 9, 2013 at 1:47 PM, Andres Freund and...@2ndquadrant.com wrote: On 2013-01-09 13:34:12 +0100, Magnus Hagander wrote: Am I the only one who finds this way of posting patches really annoying? Well, I unsurprisingly don't ;) Yeah, that's not surprising :) Here is a patch with no description other than a list of changed files. And discussion happens in a completely different email. They contain the commit message - which in most of the cases is more informative than the one just posted, which was definitely rather short. It should like in e.g. http://archives.postgresql.org/message-id/1352942234-3953-11-git-send-email-andres%402ndquadrant.com They are really two different issues - the posting a patch without a description, and the separation of threads. It's when they are combined together that it becomes *really* annoying :) When it'sposted as a separate email *with* a better commit message it's at least easier to start a discussion off it. But I still find it much omre annoying than just posting the patch in-thread. What's wrong with just posting the patch as a regular attachment(s) to a regular thread, like other people do? Two issues: - If you have a bigger series of patches (like the whole logical decoding thing) posting all patches in a single mail makes the following thread even harder to follow than its currently the case. Note how even in this, far smaller, case the discussion actually happened in the appropriate subthreads. I find it way much easier to reread through an old thread that way to reassure myself what was discussed. Yes. So one thread per patch. That's what you already have. That's not a factor of how the patches are posted, that's just a factor of how many threads you break it up in. I can agree that posting 20 different patches inthe same thread is even worse :) - mhonarc does really strange things if you attach two git created patches (splits them into multiple mails) mhonarc does a lot of strange things. But this part is actually not mhonarc's fault - it's majordomo that writes them into an mbox file in a format that you can't see the difference between the patch and the different message. Heck, it quite often gets it wrong even if you just post *one* patch when it's generated by git. This is handled better by the new archives code. It may be just me. But it may be others as well, so I figured I should raise the issue :) I am happy to comply with whatever others prefer. Yeah, so far it's also just my opinion in the other direction :) Hopefully, some others will have thoughts about it too. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] askpass program for libpq
I would like to have something like ssh-askpass for libpq. The main reason is that I don't want to have passwords in plain text on disk, even if .pgpass is read protected. By getting the password from an external program, I can integrate libpq tools with the host system's key chain or wallet thing, which stores passwords encrypted. I'm thinking about adding a new connection option askpass with environment variable PGASKPASS. One thing I haven't quite figured out is how to make this ask for passwords only if needed. Maybe it needs two connection options, one to say which program to use and one to say whether to use it. Ideas? -- 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 1/2] Provide a common malloc wrappers and palloc et al. emulation for frontend'ish environs
On Wed, Jan 9, 2013 at 9:54 PM, Magnus Hagander mag...@hagander.net wrote: On Wed, Jan 9, 2013 at 1:47 PM, Andres Freund and...@2ndquadrant.com wrote: Yeah, so far it's also just my opinion in the other direction :) Hopefully, some others will have thoughts about it too. Just giving my 2c here... Instead of posting multiple 5~7 patches at the same time, why not limiting the number of patches published at the same time to a lower number (max 2~3)? The logical replication implementation can be surely broken down into many more pieces that could be reviewed carefully one by one, and in a way that would make the implementation steps clearer than it is now for all the people of this ML. OK this would make the review process longer but the good point is that some hackers who are only specialized in some areas of the PG code would be able to give precious feedback. -- Michael Paquier http://michael.otacoo.com
Re: [HACKERS] Extra XLOG in Checkpoint for StandbySnapshot
On Wednesday, January 09, 2013 5:49 PM Andres Freund wrote:
On 2013-01-09 15:06:04 +0530, Amit Kapila wrote:
On Wednesday, January 09, 2013 2:28 PM Andres Freund wrote:
On 2013-01-09 14:04:32 +0530, Amit Kapila wrote:
On Tuesday, January 08, 2013 8:57 PM Andres Freund wrote:
On 2013-01-08 20:33:28 +0530, Amit Kapila wrote:
On Tuesday, January 08, 2013 8:01 PM Andres Freund wrote:
On 2013-01-08 19:51:39 +0530, Amit Kapila wrote:
On Monday, January 07, 2013 7:15 PM Andres Freund wrote:
On 2013-01-07 19:03:35 +0530, Amit Kapila wrote:
On Monday, January 07, 2013 6:30 PM Simon Riggs
wrote:
On 7 January 2013 12:39, Amit Kapila
amit.kap...@huawei.com
wrote:
The information that no transactions are currently
running
allows
you
to
build a recovery snapshot, without that information the
standby
won't
start answering queries. Now that doesn't matter if all
standbys
already
have built a snapshot, but the primary cannot know
that.
Can't we make sure that checkpoint operation doesn't
happen
for
below
conds.
a. nothing has happened during or after last checkpoint
OR
b. nothing except snapshotstanby WAL has happened
Currently it is done for point a.
Having to issue a checkpoint while ensuring
transactions
are
running
just to get a standby up doesn't seem like a good idea
to
me :)
Simon:
If you make the correct test, I'd be more inclined to
accept
the
premise.
Not sure, what exact you are expecting from test?
The test is do any one operation on system and then keep
the
system
idle.
Now at each checkpoint interval, it logs WAL for
SnapshotStandby.
I can't really follow what you want to do here. The
snapshot is
only
logged if a checkpoint is performed anyway? As recovery
starts
at
(the
logical) checkpoint's location we need to log a snapshot
exactly
there. If you want to avoid activity when the system is
idle
you
need
to
prevent checkpoints from occurring itself.
Even if the checkpoint is scheduled, it doesn't perform
actual
operation if
there's nothing logged between
current and previous checkpoint due to below check in
CreateCheckPoint()
function.
if (curInsert == ControlFile-checkPoint +
MAXALIGN(SizeOfXLogRecord +
sizeof(CheckPoint))
ControlFile-checkPoint ==
ControlFile-checkPointCopy.redo)
But if we set the wal_level as hot_standby, it will log
snapshot,
now
next
time again when function CreateCheckPoint()
will get called due to scheduled checkpoint, the above check
will
fail and
it will again log snapshot, so this will continue, even if
the
system
is
totally idle.
I understand that it doesn't cause any problem, but I think
it is
better if
the repeated log of snapshot in this scenario can be avoided.
ISTM in that case you just need a way to cope with the
additionally
logged record in the above piece of code. Not logging seems to
be
the
entirely wrong way to go at this.
I think one of the ways code can be modified is as below:
+ /*size of running transactions log when there is
no
active transation*/
+if (!shutdown XLogStandbyInfoActive())
+{
+runningXactXLog =
MAXALIGN(MinSizeOfXactRunningXacts) + SizeOfXLogRecord;
+}
!if (curInsert == ControlFile-checkPoint +
!MAXALIGN(SizeOfXLogRecord +
sizeof(CheckPoint))
!ControlFile-checkPoint ==
ControlFile-checkPointCopy.redo)
!if (curInsert == ControlFile-checkPoint +
!MAXALIGN(SizeOfXLogRecord +
sizeof(CheckPoint))
!ControlFile-checkPoint ==
ControlFile-checkPointCopy.redo + runningXactXLog)
Second condition is checking the last checkpoint WAL position
with
the
current one.
Since ControlFile-checkPointCopy.redo holds the value before
running
Xact WAL was inserted
and ControlFile-checkPoint holds the value after running Xact
WAL
got
inserted, so if no new WAL was inserted apart from running
Xacts
and
Checkpoint WAL, then this condition will be true.
I don't think thats safe, there could have been another record
inserted
that happens to be MinSizeOfXactRunningXacts big and we would still
skip the checkpoint.
I think such can happen
Re: [HACKERS] [PATCH 1/2] Provide a common malloc wrappers and palloc et al. emulation for frontend'ish environs
On 2013-01-09 22:23:25 +0900, Michael Paquier wrote: On Wed, Jan 9, 2013 at 9:54 PM, Magnus Hagander mag...@hagander.net wrote: On Wed, Jan 9, 2013 at 1:47 PM, Andres Freund and...@2ndquadrant.com wrote: Yeah, so far it's also just my opinion in the other direction :) Hopefully, some others will have thoughts about it too. Just giving my 2c here... Instead of posting multiple 5~7 patches at the same time, why not limiting the number of patches published at the same time to a lower number (max 2~3)? I tried to do this. ilist, binaryheap and this this thread... ;) The logical replication implementation can be surely broken down into many more pieces that could be reviewed carefully one by one, and in a way that would make the implementation steps clearer than it is now for all the people of this ML. I don't really see any additional useful splits from the ones made last round. The relfilenode stuff could be submitted separately but thats about it and its seemingly not all that useful itself (I personally want the (tablespace, relfilenode) = reloid mapping function independently, but I seem to be alone). Its hard to get useful review for patches which don't have a patch using the facility nearby in my experience. Where do you see a useful split? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_upgrade with parallel tablespace copying
Slightly modified patch applied. This is my last planned pg_upgrade
change for 9.3.
---
On Mon, Jan 7, 2013 at 10:51:21PM -0500, Bruce Momjian wrote:
Pg_upgrade by default (without --link) copies heap/index files from the
old to new cluster. This patch implements parallel heap/index file
copying in pg_upgrade using the --jobs option. It uses the same
infrastructure used for pg_upgrade parallel dump/restore. Here are the
performance results:
--- seconds ---
GBgitpatched
2 62.0963.75
4 95.93 107.22
8 194.96 195.29
16 494.38 348.93
32 983.28 644.23
64 2227.73 1244.08
128 4735.83 2547.09
Because of the kernel cache, you only see a big win when the amount of
copy data exceeds the kernel cache. For testing, I used a 24GB, 16-core
machine with two magnetic disks with one tablespace on each. Using more
tablespaces would yield larger improvements. My test script is
attached.
I consider this patch ready for application. This is the last
pg_upgrade performance improvement idea I am considering.
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
new file mode 100644
index 59f8fd0..1780788
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
*** create_script_for_old_cluster_deletion(c
*** 606,612
fprintf(script, RMDIR_CMD %s\n,
fix_path_separator(old_cluster.pgdata));
/* delete old cluster's alternate tablespaces */
! for (tblnum = 0; tblnum os_info.num_tablespaces; tblnum++)
{
/*
* Do the old cluster's per-database directories share a
directory
--- 606,612
fprintf(script, RMDIR_CMD %s\n,
fix_path_separator(old_cluster.pgdata));
/* delete old cluster's alternate tablespaces */
! for (tblnum = 0; tblnum os_info.num_old_tablespaces; tblnum++)
{
/*
* Do the old cluster's per-database directories share a
directory
*** create_script_for_old_cluster_deletion(c
*** 621,634
/* remove PG_VERSION? */
if (GET_MAJOR_VERSION(old_cluster.major_version) = 804)
fprintf(script, RM_CMD %s%s%cPG_VERSION\n,
!
fix_path_separator(os_info.tablespaces[tblnum]),
fix_path_separator(old_cluster.tablespace_suffix),
PATH_SEPARATOR);
for (dbnum = 0; dbnum old_cluster.dbarr.ndbs; dbnum++)
{
fprintf(script, RMDIR_CMD %s%s%c%d\n,
!
fix_path_separator(os_info.tablespaces[tblnum]),
fix_path_separator(old_cluster.tablespace_suffix),
PATH_SEPARATOR,
old_cluster.dbarr.dbs[dbnum].db_oid);
}
--- 621,634
/* remove PG_VERSION? */
if (GET_MAJOR_VERSION(old_cluster.major_version) = 804)
fprintf(script, RM_CMD %s%s%cPG_VERSION\n,
!
fix_path_separator(os_info.old_tablespaces[tblnum]),
fix_path_separator(old_cluster.tablespace_suffix),
PATH_SEPARATOR);
for (dbnum = 0; dbnum old_cluster.dbarr.ndbs; dbnum++)
{
fprintf(script, RMDIR_CMD %s%s%c%d\n,
!
fix_path_separator(os_info.old_tablespaces[tblnum]),
fix_path_separator(old_cluster.tablespace_suffix),
PATH_SEPARATOR,
old_cluster.dbarr.dbs[dbnum].db_oid);
}
*** create_script_for_old_cluster_deletion(c
*** 640,646
* or a version-specific subdirectory.
*/
fprintf(script, RMDIR_CMD %s%s\n,
!
fix_path_separator(os_info.tablespaces[tblnum]),
fix_path_separator(old_cluster.tablespace_suffix));
}
--- 640,646
* or a version-specific subdirectory.
*/
fprintf(script, RMDIR_CMD
Re: [HACKERS] [PATCH 1/2] Provide a common malloc wrappers and palloc et al. emulation for frontend'ish environs
How hard is the backend hit by palloc being now an additional function call? Would it be a good idea to make it (and friends) STATIC_IF_INLINE? diff --git a/src/include/port/palloc.h b/src/include/port/palloc.h new file mode 100644 index 000..a7900bf --- /dev/null +++ b/src/include/port/palloc.h @@ -0,0 +1,19 @@ +/* + * common.h + * Common support routines for bin/scripts/ + * + * Copyright (c) 2003-2013, PostgreSQL Global Development Group + * + * src/bin/scripts/common.h + */ You forgot to update the above comment. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] askpass program for libpq
On Wed, Jan 9, 2013 at 2:17 PM, Peter Eisentraut pete...@gmx.net wrote: I would like to have something like ssh-askpass for libpq. The main reason is that I don't want to have passwords in plain text on disk, even if .pgpass is read protected. By getting the password from an external program, I can integrate libpq tools with the host system's key chain or wallet thing, which stores passwords encrypted. Sounds very useful. I'm thinking about adding a new connection option askpass with environment variable PGASKPASS. One thing I haven't quite figured out is how to make this ask for passwords only if needed. Maybe it needs two connection options, one to say which program to use and one to say whether to use it. Ideas? You could call it basically where conn-password_needed is set today. So instead of dropping it directly back to the user, call the callback, try again, and drop back to the user only if it doesn't work. That means it gets called only after the connection to the server is established, but that seems reasonable given that that's the only case when you can get a password prompt as well... You don't know the server is going to ask for a password until it gets that far. In fact, might it be interesting to allow libpq to do a simple callback for the password *as well*? to implement a password prompt directly in the application, instead of having to make multiple connections? So not just as an external command, but also availbale as a direct calback. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [PATCH 1/2] Provide a common malloc wrappers and palloc et al. emulation for frontend'ish environs
On 2013-01-09 11:45:12 -0300, Alvaro Herrera wrote: How hard is the backend hit by palloc being now an additional function call? Would it be a good idea to make it (and friends) STATIC_IF_INLINE? diff --git a/src/include/port/palloc.h b/src/include/port/palloc.h new file mode 100644 index 000..a7900bf --- /dev/null +++ b/src/include/port/palloc.h @@ -0,0 +1,19 @@ +/* + * common.h + * Common support routines for bin/scripts/ + * + * Copyright (c) 2003-2013, PostgreSQL Global Development Group + * + * src/bin/scripts/common.h + */ You forgot to update the above comment. *headbang*. Gah. I hate these comments... Will update, but won't send anything again until more changes have accumulated. Thanks! Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PL/perl should fail on configure, not make
Andrew Dunstan and...@dunslane.net writes: On 01/08/2013 10:37 PM, Tom Lane wrote: We could try adding an AC_TRY_LINK test using perl_embed_ldflags, but given the weird stuff happening to redefine that value on Windows in plperl/GNUmakefile I think there's a serious risk of breaking Cygwin builds. Since I lack access to either Cygwin or a platform on which there's a problem today, I'm not going to be the one to mess with it. ITYM Mingw - the Makefile doesn't do anything for Cygwin. OK, sorry. If you want to build a configure test, you could make it conditional on the PORTNAME not being win32, since we don't seem to have a problem there anyway. Actually, if we were to try to clean this up, I'd suggest moving that logic into the configure script --- it's not apparent to me why it's a good idea to be changing configure-determined values in the Makefile. But in any case this would have to be done by somebody who's in a position to test on affected platforms. 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 1/2] Provide a common malloc wrappers and palloc et al. emulation for frontend'ish environs
On 2013-01-09 11:45:12 -0300, Alvaro Herrera wrote: How hard is the backend hit by palloc being now an additional function call? Would it be a good idea to make it (and friends) STATIC_IF_INLINE? Missed this at first... I don't think there's any measurable hit now as there is no additional function call as I chose to directly do the work directly in palloc[0]. That causes a minor amount of code duplication but imo thats ok. I didn't do that for pstrdup() but it would be easy enough to do it there as well, but I don't think it matters there. In a quick test I couldn't find any performance difference. I don't think making them STATIC_IF_INLINE functions would help as I think that would still require the CurrentMemoryContext symbol to be visible in the callers context. FWIW I previously measured whether the function call overhead via mcxt.c is measurable and it wasn't... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PL/perl should fail on configure, not make
* Christoph Berg wrote: Re: Tom Lane 2013-01-09 9802.1357702...@sss.pgh.pa.us and Python.h. However, adding one won't fix your problem on Debian-based distros, because for some wacko reason they put the headers and the shlib .so symlink in different packages, cf http://packages.debian.org/squeeze/amd64/perl/filelist http://packages.debian.org/squeeze/amd64/libperl-dev/filelist I am unfamiliar with a good reason for doing that. (I can certainly see segregating the .a static library, or even not shipping it at all, but what's it save to leave out the .so symlink?) Because the .so symlink is only needed at build time. At runtime, you need the .so.5.14 file. Hence .so.* - $pkg, .h .a .so - $pkg-dev. That was Tom's point, I believe -- Debian does not do it that way. - perl-base has /usr/lib/libperl.so.5.etc - perl has the headers and a dependency on perl-base - libperl-dev has the symlink /usr/lib/libperl.so libperl.so.5.etc So by installing perl, you get enough to satisfy configure, but there is still no usable -lperl. -- Christian -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] segmentation fault in pg_basebackup
Hi, The pg_basebackup in HEAD caused a segmentation fault in my box. $ pg_basebackup -D mmm Segmentation fault: 11 The cause is that WriteRecoveryConf() is wrongly called even if -R option is not specified in pg_basebackup. Attached patch fixes this problem. Regards, -- Fujii Masao pg_basebackup_segv_bugfix_v1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] segmentation fault in pg_basebackup
On Wed, Jan 9, 2013 at 4:56 PM, Fujii Masao masao.fu...@gmail.com wrote: Hi, The pg_basebackup in HEAD caused a segmentation fault in my box. $ pg_basebackup -D mmm Segmentation fault: 11 The cause is that WriteRecoveryConf() is wrongly called even if -R option is not specified in pg_basebackup. Attached patch fixes this problem. Ugh, my fault. Failure when merging my changes with those from Zoltan. Applied, thanks. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PL/perl should fail on configure, not make
Christian Ullrich ch...@chrullrich.net writes: * Christoph Berg wrote: Re: Tom Lane 2013-01-09 9802.1357702...@sss.pgh.pa.us I am unfamiliar with a good reason for doing that. (I can certainly see segregating the .a static library, or even not shipping it at all, but what's it save to leave out the .so symlink?) Because the .so symlink is only needed at build time. At runtime, you need the .so.5.14 file. Hence .so.* - $pkg, .h .a .so - $pkg-dev. That was Tom's point, I believe -- Debian does not do it that way. - perl-base has /usr/lib/libperl.so.5.etc - perl has the headers and a dependency on perl-base - libperl-dev has the symlink /usr/lib/libperl.so libperl.so.5.etc So by installing perl, you get enough to satisfy configure, but there is still no usable -lperl. Right. [ dons red fedora for packaging purposes... ] The beef that I've got with this is that there is no sane reason to do it like that. If you are building C code against a library, you probably need both some headers and the .so symlink, neither of which will be needed at runtime; which is why both of those typically go into a foo-dev or foo-devel subpackage. There are some other situations involving dynamic loading where you might need the .so symlink at runtime, but in that case you typically end up deciding that the symlink had better be in the base package (Debian seems to have chosen this path for Python for instance). I'm not terribly familiar with building of Perl extensions, but it seems possible that there's some use for Perl's C headers in that process, which might explain why the headers are in perl and not a perl-dev subpackage. But since the symlink requires no space to speak of, the sensible thing to do with it would have been to include it in perl along with the headers. The libperl-dev package, as constituted, doesn't make any sense: it's got the symlink which people need, and a very large static (.a) library that most people don't need. Even worse, you can't tell without close inspection which of those files is actually used by a package that requires libperl-dev, and that is something that's important to know. Under Red Hat packaging rules, the .a library likely wouldn't be shipped at all; or if there was any demand for it, it would be all by its lonesome in a package named something like libperl-static, so that it'd be easy to tell which packages actually use it. (Think about what happens if a security update needs to be made in that library ... how do you tell what has to be rebuilt? And in any case, discouraging use of the static library will reduce the number of packages to be rebuilt.) So IMO we're looking at some pretty broken packaging decisions here. I doubt we'll get Debian to change it, so I don't mind if someone wants to add a separate configure probe to verify libperl.so is available --- but as I said upthread, it's not going to be me, because I'm not in a position to test on the platforms that would be affected. 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 1/2] Provide a common malloc wrappers and palloc et al. emulation for frontend'ish environs
Magnus Hagander mag...@hagander.net writes: On Wed, Jan 9, 2013 at 1:47 PM, Andres Freund and...@2ndquadrant.com wrote: On 2013-01-09 13:34:12 +0100, Magnus Hagander wrote: Am I the only one who finds this way of posting patches really annoying? Well, I unsurprisingly don't ;) Yeah, that's not surprising :) I'm with Magnus. This is seriously annoying, especially when the discussion thread has a title not closely related to the patch emails. (It doesn't help any that the list server doesn't manage to deliver the emails in order, at least not to me --- more often than not, they're spread out over a few minutes and interleaved with other messages.) I also don't find the argument that the commit messages are a substitute for patch descriptions to be worth anything. Commit messages are, or should be, for a different audience: they are for whoever writes the release notes, or for historical purposes when someone is looking for which patches touched a particular area?. That's not the same as explaining/justifying the patch for review purposes. Reviewers want a lot more depth than is appropriate in a commit message. (TBH, I rarely use any submitter's proposed commit message anyway, but that's just me.) I'd prefer posting a single message with the discussion and the patch(es). If you think it's helpful to split a patch into separate parts for reviewing, add multiple attachments. But my experience is that such separation isn't nearly as useful as you seem to think. 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] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Andres Freund and...@2ndquadrant.com writes: On 2013-01-09 13:46:53 +0200, Heikki Linnakangas wrote: I don't understand the need for this change. Can't you just: #define palloc(s) pg_malloc(s) in the frontend context? Yes, that would be possible, but imo its the inferior solution: I'm with Heikki: in fact, I will go farther and say that this approach is DOA. The only way you get to change the backend's definition of palloc is if you can show that doing so yields a performance boost in the backend. Otherwise, we use a macro or whatever is necessary to allow frontend code to have a different underlying implementation of palloc(x). * it precludes ever sharing code without compiling twice That's never going to happen anyway, or at least there are plenty more problems in the way of it. * removing allows us to get rid of the following ugliness in dirmod.c: I agree that what dirmod is doing is pretty ugly, but it's localized enough that I don't care particularly. (Really, the only reason it's a hack and not The Solution is that at the moment there's only one file doing it that way. A trampoline function for use only by code that needs to work in both frontend and backend isn't an unreasonable 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] [PATCH] Patch to fix missing libecpg_compat.lib and libpgtypes.lib.
On Tue, Jan 8, 2013 at 4:16 AM, Peter Eisentraut pete...@gmx.net wrote: On Mon, 2012-11-19 at 14:03 +0800, JiangGuiqing wrote: hi I install postgresql-9.1.5 from source code on windows successfully. But there is not libecpg_compat.lib and libpgtypes.lib in the installed lib directory . If someone used functions of pgtypes or ecpg_compat library in ecpg programs,the ecpg program build will fail. So i modify src/tools/msvc/Install.pm to resolve this problem. The diff file refer to the attachment Install.pm.patch. Could someone with access to Windows verify this patch? Thta version doesn't even apply to 9.1 - only to master. And the indent on master is really bad. That said, function looks good, so I've cleaned i tup and applied. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.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] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
On 2013-01-09 11:37:46 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2013-01-09 13:46:53 +0200, Heikki Linnakangas wrote: I don't understand the need for this change. Can't you just: #define palloc(s) pg_malloc(s) in the frontend context? Yes, that would be possible, but imo its the inferior solution: I'm with Heikki: in fact, I will go farther and say that this approach is DOA. The only way you get to change the backend's definition of palloc is if you can show that doing so yields a performance boost in the backend. Otherwise, we use a macro or whatever is necessary to allow frontend code to have a different underlying implementation of palloc(x). Whats the usecase for being able to redefine it after the patch? Its not like you can do anything interesting given that pfree() et. al. is not a macro. * removing allows us to get rid of the following ugliness in dirmod.c: I agree that what dirmod is doing is pretty ugly, but it's localized enough that I don't care particularly. (Really, the only reason it's a hack and not The Solution is that at the moment there's only one file doing it that way. A trampoline function for use only by code that needs to work in both frontend and backend isn't an unreasonable idea.) Well, after the patch the trampoline function would be palloc() itself. Greetings, Andres Freund -- 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 1/2] Provide a common malloc wrappers and palloc et al. emulation for frontend'ish environs
On 9 January 2013 16:27, Tom Lane t...@sss.pgh.pa.us wrote: I'm with Magnus. This is seriously annoying, especially when the discussion thread has a title not closely related to the patch emails. (It doesn't help any that the list server doesn't manage to deliver the emails in order, at least not to me --- more often than not, they're spread out over a few minutes and interleaved with other messages.) I agree. I actually go to some lengths to coordinate all of these threads during review, and I'd prefer not to have to. I would have said something, but it seemed unimportant (relative to the patchset itself). -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [PATCH 1/2] Provide a common malloc wrappers and palloc et al. emulation for frontend'ish environs
On 2013-01-09 11:27:46 -0500, Tom Lane wrote: Magnus Hagander mag...@hagander.net writes: On Wed, Jan 9, 2013 at 1:47 PM, Andres Freund and...@2ndquadrant.com wrote: On 2013-01-09 13:34:12 +0100, Magnus Hagander wrote: Am I the only one who finds this way of posting patches really annoying? Well, I unsurprisingly don't ;) Yeah, that's not surprising :) I'm with Magnus. This is seriously annoying, especially when the discussion thread has a title not closely related to the patch emails. (It doesn't help any that the list server doesn't manage to deliver the emails in order, at least not to me --- more often than not, they're spread out over a few minutes and interleaved with other messages.) Ok, most seem to have a clear preference, so I won't do so anymore. I also don't find the argument that the commit messages are a substitute for patch descriptions to be worth anything. Commit messages are, or should be, for a different audience: they are for whoever writes the release notes, or for historical purposes when someone is looking for which patches touched a particular area?. That's not the same as explaining/justifying the patch for review purposes. Reviewers want a lot more depth than is appropriate in a commit message. Aggreed that they have different audiences. (TBH, I rarely use any submitter's proposed commit message anyway, but that's just me.) I noticed ;) I'd prefer posting a single message with the discussion and the patch(es). If you think it's helpful to split a patch into separate parts for reviewing, add multiple attachments. But my experience is that such separation isn't nearly as useful as you seem to think. Well, would it have been better if xlog reading, ilist, binaryheap, this cleanup, etc. have been in the same patch? They have originated out of the same work... Even the splitup in this thread seems to have helped as youve jumped on the patches where you could give rather quick input (static relpathbackend(), central Assert definitions), probably without having read the xlogreader patch itself... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Andres Freund and...@anarazel.de writes: On 2013-01-09 11:37:46 -0500, Tom Lane wrote: I agree that what dirmod is doing is pretty ugly, but it's localized enough that I don't care particularly. (Really, the only reason it's a hack and not The Solution is that at the moment there's only one file doing it that way. A trampoline function for use only by code that needs to work in both frontend and backend isn't an unreasonable idea.) Well, after the patch the trampoline function would be palloc() itself. My objection is that you're forcing *all* backend code to go through the trampoline. That probably has a negative impact on performance, and if you think it'll get committed without a thorough proof that there's no such impact, you're mistaken. Perhaps you're not aware of exactly how hot a hot-spot palloc is? It's not unlikely that this patch could hurt backend performance by several percent all by itself. Now on the other side of the coin, it's also possible that it's a wash, particularly on machines where fetching a global variable is a relatively expensive thing to do. But the driving force behind any proposed change to the backend's palloc mechanism has to be performance, and nothing but. Convenience for a patch such as this not only doesn't trump performance, I'm unwilling to grant it any standing 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] Event Triggers: adding information
Dimitri Fontaine escribió: Alvaro Herrera alvhe...@2ndquadrant.com writes: Given the OID, we use the ObjectProperty[] structure to know which cache or catalog to scan in order to get the name and namespace of the object. The changes to make ObjectPropertyType visible to code outside objectaddress.c look bogus to me. I think we should make this new code use the interfaces we already have. We need to move the new fonction get_object_name_and_namespace() into the objectaddress.c module then, and decide about returning yet another structure (because we have two values to return) or to pass that function a couple of char **. Which devil do you prefer? I made some changes to this, and I think the result (attached) is cleaner overall. Now, this review is pretty much unfinished as far as I am concerned; mainly I've been trying to figure out how it all works and improving some stuff as I go, and I haven't looked at the complete patch yet. We might very well doing some things quite differently; for example I'm not really sure of the way we handle CommandTags, or the way we do object lookups at the event trigger stage, only to have it repeated later when the actual deletion takes place. In particular, since event_trigger.c does not know the lock strength that's going to be taken at deletion, it only grabs ShareUpdateExclusiveLock; so there is lock escalation here which could lead to deadlocks. This might need to be rethought. I added a comment somewhere, noting that perhaps it's ProcessUtility's job to set the object classId (or at least the ObjectType) at the same time the TargetOid is being set, rather than have event_trigger.c figure it out from the parse node. And so on. I haven't looked at plpgsql or pg_dump yet, either. We've been discussing on IM the handling of DROP in general. The current way it works is that the object OID is reported only if the toplevel command specifies to drop a single object (so no OID if you do DROP TABLE foo,bar); and this is all done by standard_ProcessUtility. This seems bogus to me, and it's also problematic for things like DROP SCHEMA, as well as DROP OWNED BY (which, as noted upthread, is not handled at all but should of course be). I think the way to do it is have performDeletion and performMultipleDeletions (dependency.c) call into the event trigger code, by doing something like this: 1. look up all objects to drop (i.e. resolve the list of objects specified by the user, and complete with objects dependent on those) 2. iterate over this list, firing DROP at-start event triggers 3. do the actual deletion of objects (i.e. deleteOneObject, I think) 4. iterate again over the list firing DROP at-end event triggers We would have one or more event triggers being called with a context of TOPLEVEL (for objects directly mentioned in the command), and then some more calls with a context of CASCADE. Exactly how should DROP OWNED BY be handled is not clear; perhaps we should raise one TOPLEVEL event for the objects directly owned by the role? Maybe we should just do CASCADE for all objects? This is not clear yet. Thoughts? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services event_trigger_infos.7.alvherre1.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
[HACKERS] Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
On 2013-01-09 11:57:35 -0500, Tom Lane wrote: Andres Freund and...@anarazel.de writes: On 2013-01-09 11:37:46 -0500, Tom Lane wrote: I agree that what dirmod is doing is pretty ugly, but it's localized enough that I don't care particularly. (Really, the only reason it's a hack and not The Solution is that at the moment there's only one file doing it that way. A trampoline function for use only by code that needs to work in both frontend and backend isn't an unreasonable idea.) Well, after the patch the trampoline function would be palloc() itself. My objection is that you're forcing *all* backend code to go through the trampoline. That probably has a negative impact on performance, and if you think it'll get committed without a thorough proof that there's no such impact, you're mistaken. Perhaps you're not aware of exactly how hot a hot-spot palloc is? It's not unlikely that this patch could hurt backend performance by several percent all by itself. There is no additional overhead except some minor increase in code size. If you look at the patch palloc() now simply directly calls CurrentMemoryContext-methods-alloc. So there is no additional function call relative to the previous state. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PL/perl should fail on configure, not make
On 1/9/13 10:30 AM, Christian Ullrich wrote: * Christoph Berg wrote: Re: Tom Lane 2013-01-09 9802.1357702...@sss.pgh.pa.us and Python.h. However, adding one won't fix your problem on Debian-based distros, because for some wacko reason they put the headers and the shlib .so symlink in different packages, cf http://packages.debian.org/squeeze/amd64/perl/filelist http://packages.debian.org/squeeze/amd64/libperl-dev/filelist I am unfamiliar with a good reason for doing that. (I can certainly see segregating the .a static library, or even not shipping it at all, but what's it save to leave out the .so symlink?) Because the .so symlink is only needed at build time. At runtime, you need the .so.5.14 file. Hence .so.* - $pkg, .h .a .so - $pkg-dev. That was Tom's point, I believe -- Debian does not do it that way. - perl-base has /usr/lib/libperl.so.5.etc - perl has the headers and a dependency on perl-base - libperl-dev has the symlink /usr/lib/libperl.so libperl.so.5.etc So by installing perl, you get enough to satisfy configure, but there is still no usable -lperl. The reason it's like that is that the header files are usable for building Perl extensions, but that doesn't need the library. And the expectation is that if you want to build against libfoo, you install libfoo-dev. The fact that some other package also gives you half of what you need is a coincidence. The blame, if you want to assign any, is with configure, because it assumes that the header files and the library are an atomic unit and checks for the former and assumes the latter must be there as well. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PL/perl should fail on configure, not make
On 1/9/13 11:00 AM, Tom Lane wrote: The libperl-dev package, as constituted, doesn't make any sense: it's got the symlink which people need, and a very large static (.a) library that most people don't need. Even worse, you can't tell without close inspection which of those files is actually used by a package that requires libperl-dev, and that is something that's important to know. The expectation is that if you want to link against libfoo, you install libfoo-dev, and after that you can uninstall it. What's wrong with that? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] PostgreSQL hackfest @ Developer Conference 2013, Brno, CZ
Hi guys, let me announce PostgreSQL hackfest, which is held during Developer Conference 2013. Everyone from users, admins to hackers is welcome. Few words about Developer Conference: Developer Conference is an yearly conference for all Linux and JBoss Developers, Admins and Linux users organized by Red Hat Czech Republic, the Fedora and JBoss Community. 2012 DevConf had 60 talks in 3 different tracks and additional to that 3 more rooms with workshops and hackfests. With around 600 participants DevConf.cz is one of the biggest events about free software in the Czech Republic. You might check out photos, videos and blogposts about past DevConf.cz. This year the conference is held on February 23rd and 24th (Saturday and Sunday). What to expect about PostgreSQL hackfest event itself: PostgreSQL hackfest is one of the events at Developer Conference 2013, which is held on Sunday (2nd day of the conference) from 9am. Everyone around PostgreSQL project from users, admins to hackers is invited to meet us and talk about current PostgreSQL topics, issues or plans for the future. If we don't want just to talk anymore, we can hack together on some hot issues, so don't forget your laptop. See the links for details: G+ hackfest event: https://plus.google.com/u/0/events/cqb5364p4377vd8lp6gvmqqtk6c DevConf web: http://www.devconf.cz/ DevConf schedule: http://developerconference2013.sched.org/ Contact person: Honza Horak (hho...@redhat.com) Feel free to share this announcement. See you! Honza -- 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] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Andres Freund and...@anarazel.de writes: On 2013-01-09 11:57:35 -0500, Tom Lane wrote: My objection is that you're forcing *all* backend code to go through the trampoline. That probably has a negative impact on performance, and if you think it'll get committed without a thorough proof that there's no such impact, you're mistaken. Perhaps you're not aware of exactly how hot a hot-spot palloc is? It's not unlikely that this patch could hurt backend performance by several percent all by itself. There is no additional overhead except some minor increase in code size. If you look at the patch palloc() now simply directly calls CurrentMemoryContext-methods-alloc. So there is no additional function call relative to the previous state. Apparently we're failing to communicate, so let me put this as clearly as possible: an unsupported assertion that this patch has zero cost isn't worth the electrons it's written on. We're talking about a place where single instructions can make a large difference. I see that you've avoided an extra level of function call by duplicating code, but there are (at least) two reasons why that could be a loser: * now there are two hotspots not one, ie both MemoryContextAlloc and palloc will be competing for L1 cache, likewise for MemoryContextAllocZero and palloc0; * depending on machine architecture and compiler optimizations, multiple fetches of the global variable CurrentMemoryContext are quite likely to cost more than fetching it once into a parameter register. As I said, it's possible that this is a wash. But it's very possible that it isn't. In any case it's not clear to me why duplicating code like that is a less ugly approach than having different macro definitions for frontend and backend. 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] Further pg_upgrade analysis for many tables
On Wed, Jan 9, 2013 at 3:59 AM, Simon Riggs si...@2ndquadrant.com wrote: On 9 November 2012 18:50, Jeff Janes jeff.ja...@gmail.com wrote: quadratic behavior in the resource owner/lock table I didn't want to let that particular phrase go by without saying exactly what behaviour is that?, so we can discuss fixing that also. It is the thing that was fixed in commit eeb6f37d89fc60c6449ca1, Add a small cache of locks owned by a resource owner in ResourceOwner. But that fix is only in 9.3devel. Cheers, Jeff -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PL/perl should fail on configure, not make
Peter Eisentraut pete...@gmx.net writes: On 1/9/13 11:00 AM, Tom Lane wrote: The libperl-dev package, as constituted, doesn't make any sense: it's got the symlink which people need, and a very large static (.a) library that most people don't need. Even worse, you can't tell without close inspection which of those files is actually used by a package that requires libperl-dev, and that is something that's important to know. The expectation is that if you want to link against libfoo, you install libfoo-dev, and after that you can uninstall it. What's wrong with that? What's wrong is that it's hard to tell whether the resulting package will contain a reference to the shared library (libperl.so.whatever) or an embedded copy of the static library. As I tried to explain, this is something that a well-run distro will want to be able to control, or at least determine automatically from the package's BuildRequires list (RPM-ism, not sure what Debian's package management stuff calls the equivalent concept). That makes it a bad idea independently of the problem of whether two configure tests are needed rather than one. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: Set effective_cache_size to greater of .conf value, shared_buffers
Tom Lane t...@sss.pgh.pa.us writes: Well, the problem of find out the box's physical RAM is doubtless solvable if we're willing to put enough sweat and tears into it, but I'm dubious that it's worth the trouble. The harder part is how to know if the box is supposed to be dedicated to the database. Bear in mind that the starting point of this debate was the idea that we're talking about an inexperienced DBA who doesn't know about any configuration knob we might provide for the purpose. It seems to me that pgfincore has the smarts we need to know about that, and that Cédric has code and refenrences for making it work on all platforms we care about (linux, bsd, windows for starters). Then we could have a pretty decent snapshot of the information, and maybe a background worker of some sort would be able to refresh the information while the server is running. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
On 2013-01-09 12:32:20 -0500, Tom Lane wrote: Andres Freund and...@anarazel.de writes: On 2013-01-09 11:57:35 -0500, Tom Lane wrote: My objection is that you're forcing *all* backend code to go through the trampoline. That probably has a negative impact on performance, and if you think it'll get committed without a thorough proof that there's no such impact, you're mistaken. Perhaps you're not aware of exactly how hot a hot-spot palloc is? It's not unlikely that this patch could hurt backend performance by several percent all by itself. There is no additional overhead except some minor increase in code size. If you look at the patch palloc() now simply directly calls CurrentMemoryContext-methods-alloc. So there is no additional function call relative to the previous state. Apparently we're failing to communicate, so let me put this as clearly as possible: an unsupported assertion that this patch has zero cost isn't worth the electrons it's written on. Well, I *did* benchmark it as noted elsewhere in the thread, but thats obviously just machine (E5520 x 2) with one rather restricted workload (pgbench -S -jc 40 -T60). At least its rather palloc heavy. Here are the numbers: before: #101646.763208 101350.361595 101421.425668 101571.211688 101862.172051 101449.857665 after: #101553.596257 102132.277795 101528.816229 101733.541792 101438.531618 101673.400992 So on my system if there is a difference, its positive (0.12%). In any case it's not clear to me why duplicating code like that is a less ugly approach than having different macro definitions for frontend and backend. Imo its better abstracted and more consistent (pfree() is not a macro, palloc() is) and actually makes it easier to redirect code somewhere else. It also opens the door for exposing less knowledge in utils/palloc.h. Anyway, we can use a macro instead, I don't think its that important. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: Set effective_cache_size to greater of .conf value, shared_buffers
Dimitri, It seems to me that pgfincore has the smarts we need to know about that, and that Cédric has code and refenrences for making it work on all platforms we care about (linux, bsd, windows for starters). Well, fincore is Linux-only, and for that matter only more recent versions of linux. It would be great if we could just detect the RAM, but then we *still* have to ask the user if this is a dedicated box or not. Then we could have a pretty decent snapshot of the information, and maybe a background worker of some sort would be able to refresh the information while the server is running. I don't see a background worker as necessary or even desirable; people don't frequently add RAM to their systems. A manually callable command would be completely adequate. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: Set effective_cache_size to greater of .conf value, shared_buffers
On Wed, Jan 9, 2013 at 3:39 PM, Josh Berkus j...@agliodbs.com wrote: It seems to me that pgfincore has the smarts we need to know about that, and that Cédric has code and refenrences for making it work on all platforms we care about (linux, bsd, windows for starters). Well, fincore is Linux-only, and for that matter only more recent versions of linux. It would be great if we could just detect the RAM, but then we *still* have to ask the user if this is a dedicated box or not. Not really. I'm convinced, and not only for e_c_s, that autoconfiguration is within the realm of possibility. However, since such a statement holds little weight without a patch attached, I've been keeping it to myself (until I can invest the effort needed for that patch). In any case, as eavesdroppers can infer a cryptographic key by timing operations or measuring power consumption, I'm pretty sure postgres can infer cost metrics and/or time sharing with clever instrumentation. The trick lies in making such instrumentation uninstrusive. -- 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] proposal: Set effective_cache_size to greater of .conf value, shared_buffers
Claudio, Not really. I'm convinced, and not only for e_c_s, that autoconfiguration is within the realm of possibility. Hey, if you can do it, my hat's off to you. In any case, as eavesdroppers can infer a cryptographic key by timing operations or measuring power consumption, I'm pretty sure postgres can infer cost metrics and/or time sharing with clever instrumentation. The trick lies in making such instrumentation uninstrusive. ... and not requiring a great deal of code maintenance for each and every release of Linux and Windows. Anyway, we could do something for 9.3 if we just make available RAM a manual setting. Asking the user how much RAM is available for Postgres is not a terribly difficult question. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [PATCH 1/2] Provide a common malloc wrappers and palloc et al. emulation for frontend'ish environs
Andres Freund and...@2ndquadrant.com writes: On 2013-01-09 11:27:46 -0500, Tom Lane wrote: I'd prefer posting a single message with the discussion and the patch(es). If you think it's helpful to split a patch into separate parts for reviewing, add multiple attachments. But my experience is that such separation isn't nearly as useful as you seem to think. Well, would it have been better if xlog reading, ilist, binaryheap, this cleanup, etc. have been in the same patch? They have originated out of the same work... Even the splitup in this thread seems to have helped as youve jumped on the patches where you could give rather quick input (static relpathbackend(), central Assert definitions), probably without having read the xlogreader patch itself... No, I agree that global-impact things like this palloc rearrangement are much better proposed and debated separately than as part of something like xlogreader. What I was reacting to was the specific patch set associated with this thread. I don't see the point of breaking out a two-line sub-patch such as you did in http://archives.postgresql.org/message-id/1357730830-25999-3-git-send-email-and...@2ndquadrant.com 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] PL/Python: Add spidata to all spiexceptions
On 12/12/12 20:21, Karl O. Pinc wrote: There were 2 outstanding issue raised: Is this useful enough/proceeding in the right direction to commit now? Should some of the logic be moved out of a subroutine and into the calling code? I can see arguments to be made for both sides. I'm asking that you think it through to the extent you deem necessary and make changes or not. At that point it should be ready to send to a committer. (I'll re-test first, if you make any changes.) Oh my, I have dropped the ball on this one in the worst manner possible. Sorry! I actually still prefer to keep the signatures of PLy_get_spi_sqlerrcode and PLy_get_spi_error_data similar, so I'd opt for keeping the patch as-is :) Thanks, 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] Re: [PATCH 1/2] Provide a common malloc wrappers and palloc et al. emulation for frontend'ish environs
Tom Lane t...@sss.pgh.pa.us schrieb: Andres Freund and...@2ndquadrant.com writes: On 2013-01-09 11:27:46 -0500, Tom Lane wrote: I'd prefer posting a single message with the discussion and the patch(es). If you think it's helpful to split a patch into separate parts for reviewing, add multiple attachments. But my experience is that such separation isn't nearly as useful as you seem to think. Well, would it have been better if xlog reading, ilist, binaryheap, this cleanup, etc. have been in the same patch? They have originated out of the same work... Even the splitup in this thread seems to have helped as youve jumped on the patches where you could give rather quick input (static relpathbackend(), central Assert definitions), probably without having read the xlogreader patch itself... No, I agree that global-impact things like this palloc rearrangement are much better proposed and debated separately than as part of something like xlogreader. What I was reacting to was the specific patch set associated with this thread. I don't see the point of breaking out a two-line sub-patch such as you did in http://archives.postgresql.org/message-id/1357730830-25999-3-git-send-email-and...@2ndquadrant.com Ah, yes. I See your point. The not all that good reasoning I had in mind was that that one should be uncontroversial as it seemed to be the only unchecked malloc call in src/bin. So it could be committed independent from the more controversial stuff... Same with the single whitespace removal patch upthread... Andres --- Please excuse brevity and formatting - I am writing this on my mobile phone. -- 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] PL/Python: Add spidata to all spiexceptions
On 01/09/2013 01:08:39 PM, Jan Urbański wrote: I can see arguments to be made for both sides. I'm asking that you think it through to the extent you deem necessary and make changes or not. At that point it should be ready to send to a committer. (I'll re-test first, if you make any changes.) Oh my, I have dropped the ball on this one in the worst manner possible. Sorry! My fault too. I've been thinking I should write to remind you. I actually still prefer to keep the signatures of PLy_get_spi_sqlerrcode and PLy_get_spi_error_data similar, so I'd opt for keeping the patch as-is :) I will send it on to a committer. Regards, Karl k...@meme.com Free Software: You don't pay back, you pay forward. -- Robert A. Heinlein -- 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] Further pg_upgrade analysis for many tables
On 9 January 2013 17:50, Jeff Janes jeff.ja...@gmail.com wrote: On Wed, Jan 9, 2013 at 3:59 AM, Simon Riggs si...@2ndquadrant.com wrote: On 9 November 2012 18:50, Jeff Janes jeff.ja...@gmail.com wrote: quadratic behavior in the resource owner/lock table I didn't want to let that particular phrase go by without saying exactly what behaviour is that?, so we can discuss fixing that also. It is the thing that was fixed in commit eeb6f37d89fc60c6449ca1, Add a small cache of locks owned by a resource owner in ResourceOwner. But that fix is only in 9.3devel. That's good, it fixes the problem I reported in 2010, under SAVEPOINTs and COMMIT performance. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] buffer assertion tripping under repeat pgbench load
On 12/23/12 3:17 PM, Simon Riggs wrote:
We already have PrintBufferLeakWarning() for this, which might be a bit neater.
It does look like basically the same info. I hacked the code to
generate this warning all the time. Patch from Andres I've been using:
WARNING: refcount of buf 1 containing global/12886 blockNum=0,
flags=0x106 is 0 should be 0, globally: 0
And using PrintBufferLeakWarning with the same input:
WARNING: buffer refcount leak: [001] (rel=global/12886, blockNum=0,
flags=0x106, refcount=0 0)
Attached is a change I'd propose be committed. This doesn't do anything
in a non-assertion build, it just follows all of the why don't you
try... suggestions I've gotten here. I still have no idea why these
are popping out for me. I'm saving the state on all this to try and
track it down again later. I'm out of time to bang my head on this
particular thing anymore right now, it doesn't seem that important given
it's not reproducible anywhere else.
Any assertion errors that come out will be more useful with just this
change though. Output looks like this (from hacked code to always trip
it and shared_buffers=16):
...
WARNING: buffer refcount leak: [016]
(rel=pg_tblspc/0/PG_9.3_201212081/0/0_(null), blockNum=4294967295,
flags=0x0, refcount=0 0)
WARNING: buffers with non-zero refcount is 16
TRAP: FailedAssertion(!(RefCountErrors == 0), File: bufmgr.c, Line:
1724)
I intentionally used a regular diff for this small one because it shows
the subtle changes I made here better. I touched more than I strictly
had to because this area of the code is filled with off by one
corrections, and adding this warning highlighted one I objected to.
Note how the function is defined:
PrintBufferLeakWarning(Buffer buffer)
The official buffer identifier description says:
typedef int Buffer;
Zero is invalid, positive is the index of a shared buffer (1..NBuffers),
negative is the index of a local buffer (-1 .. -NLocBuffer).
However, the loop in AtEOXact_Buffers aimed to iterate over
PrivateRefCount, using array indexes 0...NBuffers-1. It was using an
array index rather than a proper buffer identification number. And that
meant when I tried to pass it to PrintBufferLeakWarning, it asserted on
the buffer id--because Zero is invalid.
I could have just fixed that with an off by one fix in the other
direction. That seemed wrong to me though. All of the other code like
PrintBufferLeakWarning expects to see a Buffer value. It already
corrects for the off by one problem like this:
buf = BufferDescriptors[buffer - 1];
loccount = PrivateRefCount[buffer - 1];
It seemed cleaner to me to make the i variable in AtEOXact_Buffers be a
Buffer number. Then you access PrivateRefCount[buffer - 1] in that
code, making it look like more of the surrounding references. And the
value goes directly into PrintBufferLeakWarning without a problem.
--
Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com
diff --git a/src/backend/storage/buffer/bufmgr.c
b/src/backend/storage/buffer/bufmgr.c
index dddb6c0..3fe2e02 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1696,12 +1696,20 @@ AtEOXact_Buffers(bool isCommit)
#ifdef USE_ASSERT_CHECKING
if (assert_enabled)
{
- int i;
+ Buffer i;
+ int RefCountErrors = 0;
- for (i = 0; i NBuffers; i++)
+ for (i = 1; i = NBuffers; i++)
{
- Assert(PrivateRefCount[i] == 0);
+ if (PrivateRefCount[i - 1] != 0)
+ {
+ PrintBufferLeakWarning(i);
+ RefCountErrors++;
+ }
}
+ if (RefCountErrors 0)
+ elog(WARNING, buffers with non-zero refcount is %d,
RefCountErrors);
+ Assert(RefCountErrors == 0);
}
#endif
--
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] Index build temp files
* Tom Lane (t...@sss.pgh.pa.us) wrote: Does the user running CREATE INDEX have CREATE permission on those tablespaces? (A quick way to double check is to try to SET temp_tablespaces to that value explicitly.) The code will silently ignore any temp tablespaces you don't have such permission for. Alright, this isn't quite as open-and-shut as it may have originally seemed. We're apparently cacheing the temp tablespaces which should be used, even across set role's and security definer functions, which I would argue isn't correct. Attached are two test scripts and results from them. The gist of it is this: With the first script, a privileged user creates a temp table and this table ends up in a tablespace which that user has access to. In the same session, the user drops privileges using a 'set role' to a less privileged user (who doesn't have access to the same tablespaces) and then tries to create a temporary table- boom: permission denied error. In the second script, an unprivileged user creates a temp table, which goes into the default tablespace (this is fine- there are other temp tablespaces, but this user doesn't have access to them), but then calls a SECURITY DEFINER function, which runs as a privileged user who DOES have access to the temp tablespaces defined by default. What ends up happening, however, is that the temporary table created during the security definer function ends up going into the default tablespace instead. If the security definer function calls 'set temp_tablespaces', before creating the temp table, it'll go into the correct tablespace but after the security definer function ends, if the regular user tries to create a temporary table they'll get a permission denied error. I've not gone and looked at any of the code behind this yet (hopefully will be able to soon). It seems like we need to modify the cacheing behavior of the temp tablespace code to account for the role that is currently active. This can't work quite like search_path since we want to reuse the same tablespace, if possible, for the duration of a session, per the docs. Thoughts? Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Index build temp files
* Stephen Frost (sfr...@snowman.net) wrote: Attached are two test scripts and results from them. The gist of it is this: Now with the attachements. Apologies about that. Note that you'll need to create the temp tablespace first. Thanks, Stephen test_tblspace.sql Description: application/sql test_tblspace2.sql Description: application/sql current_user -- sfrost (1 row) role -- none (1 row) temp_tablespaces -- rn_temp_01 (1 row) List of tablespaces Name| Owner | Location | Access privileges | Description ++---+---+- rn_temp_01 | sfrost | /var/lib/postgresql/rn_tblspcs/rn_temp_01 | | (1 row) create temporary table test1 (a int); CREATE TABLE select relname,reltablespace from pg_class where relname = test1; relname | reltablespace -+--- test1 | 36578 (1 row) drop role if exists test_role; DROP ROLE create role test_role; CREATE ROLE set role test_role; SET create temporary table test3 (a int); ERROR: permission denied for tablespace rn_temp_01 current_user -- sfrost (1 row) role -- none (1 row) temp_tablespaces -- rn_temp_01 (1 row) List of tablespaces Name| Owner | Location | Access privileges | Description ++---+---+- rn_temp_01 | sfrost | /var/lib/postgresql/rn_tblspcs/rn_temp_01 | | (1 row) create function test_func () returns int language plpgsql security definer as begin create temporary table test2 (a int); return 1; end;; CREATE FUNCTION List of functions Schema | Name| Result data type | Argument data types | Type | Volatility | Owner | Language |Source code | Description +---+--+-++++--++- public | test_func | integer | | normal | volatile | sfrost | plpgsql | begin create temporary table test2 (a int); return 1; end; | (1 row) drop role if exists test_role; DROP ROLE create role test_role; CREATE ROLE set role test_role; SET create temporary table test (a int); CREATE TABLE relname | reltablespace -+--- test| 0 (1 row) test_func --- 1 (1 row) relname | reltablespace -+--- test2 | 0 (1 row) signature.asc Description: Digital signature
Re: [HACKERS] Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Andres Freund and...@2ndquadrant.com writes: Well, I *did* benchmark it as noted elsewhere in the thread, but thats obviously just machine (E5520 x 2) with one rather restricted workload (pgbench -S -jc 40 -T60). At least its rather palloc heavy. Here are the numbers: before: #101646.763208 101350.361595 101421.425668 101571.211688 101862.172051 101449.857665 after: #101553.596257 102132.277795 101528.816229 101733.541792 101438.531618 101673.400992 So on my system if there is a difference, its positive (0.12%). pgbench-based testing doesn't fill me with a lot of confidence for this --- its numbers contain a lot of communication overhead, not to mention that pgbench itself can be a bottleneck. It struck me that we have a recent test case that's known to be really palloc-intensive, namely Pavel's example here: http://www.postgresql.org/message-id/CAFj8pRCKfoz6L82PovLXNK-1JL=jzjwat8e2bd2pwnkm7i7...@mail.gmail.com I set up a non-cassert build of commit 78a5e738e97b4dda89e1bfea60675bcf15f25994 (ie, just before the patch that reduced the data-copying overhead for that). On my Fedora 16 machine (dual 2.0GHz Xeon E5503, gcc version 4.6.3 20120306 (Red Hat 4.6.3-2)) I get a runtime for Pavel's example of 17023 msec (average over five runs). I then applied oprofile and got a breakdown like this: samples| %| -- 108409 84.5083 /home/tgl/testversion/bin/postgres 13723 10.6975 /lib64/libc-2.14.90.so 3153 2.4579 /home/tgl/testversion/lib/postgresql/plpgsql.so samples %symbol name 1096010.1495 AllocSetAlloc 6325 5.8572 MemoryContextAllocZeroAligned 6225 5.7646 base_yyparse 3765 3.4866 copyObject 2511 2.3253 MemoryContextAlloc 2292 2.1225 grouping_planner 2044 1.8928 SearchCatCache 1956 1.8113 core_yylex 1763 1.6326 expression_tree_walker 1347 1.2474 MemoryContextCreate 1340 1.2409 check_stack_depth 1276 1.1816 GetCachedPlan 1175 1.0881 AllocSetFree 1106 1.0242 GetSnapshotData 1106 1.0242 _SPI_execute_plan 1101 1.0196 extract_query_dependencies_walker I then applied the palloc.h and mcxt.c hunks of your patch and rebuilt. Now I get an average runtime of 1 ms, a full 2% faster, which is a bit astonishing, particularly because the oprofile results haven't moved much: 107642 83.7427 /home/tgl/testversion/bin/postgres 14677 11.4183 /lib64/libc-2.14.90.so 3180 2.4740 /home/tgl/testversion/lib/postgresql/plpgsql.so samples %symbol name 10038 9.3537 AllocSetAlloc 6392 5.9562 MemoryContextAllocZeroAligned 5763 5.3701 base_yyparse 4810 4.4821 copyObject 2268 2.1134 grouping_planner 2178 2.0295 core_yylex 1963 1.8292 palloc 1867 1.7397 SearchCatCache 1835 1.7099 expression_tree_walker 1551 1.4453 check_stack_depth 1374 1.2803 _SPI_execute_plan 1282 1.1946 MemoryContextCreate 1187 1.1061 AllocSetFree ... 653 0.6085 palloc0 ... 552 0.5144 MemoryContextAlloc The number of calls of AllocSetAlloc certainly hasn't changed at all, so how did that get faster? I notice that the postgres executable is about 0.2% smaller, presumably because a whole lot of inlined fetches of CurrentMemoryContext are gone. This makes me wonder if my result is due to chance improvements of cache line alignment for inner loops. I would like to know if other people get comparable results on other hardware (non-Intel hardware would be especially interesting). If this result holds up across a range of platforms, I'll withdraw my objection to making palloc a plain function. 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] Index build temp files
On 9 January 2013 02:42, Tom Lane t...@sss.pgh.pa.us wrote: Stephen Frost sfr...@snowman.net writes: Here's what we're seeing: postgresql.conf: temp_tablespaces = 'temp_01,temp_02' I have temp file logging on in postgresql.conf, so here's what we see: LOG: temporary file: path base/pgsql_tmp/pgsql_tmp9221.4, size 204521472 CONTEXT: SQL statement create index table_key_idx on table (key) tablespace data_n04 We observe the files being created under $DATA/base/pgsql_tmp/, ignoring the temp tablespaces and not using the tablespace where the index is ultimately created either. Does the user running CREATE INDEX have CREATE permission on those tablespaces? (A quick way to double check is to try to SET temp_tablespaces to that value explicitly.) The code will silently ignore any temp tablespaces you don't have such permission for. I think we need to allow a TEMP permission on tablespaces, so that you aren't allowed to create normal objects but you can create TEMP objects and sort files there. I think SHOW temp_tablespaces should only show the valid tablespaces, not all of the ones actually listed in the parameter, otherwise you have no clue that the parameter setting is ineffectual for you. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Index build temp files
Stephen Frost sfr...@snowman.net writes: Alright, this isn't quite as open-and-shut as it may have originally seemed. We're apparently cacheing the temp tablespaces which should be used, even across set role's and security definer functions, which I would argue isn't correct. Ah. Yeah, that would be true. We do have mechanism that forces search_path to be recomputed across changes of active role, but it's expensive to do that, and it seems of rather debatable value to do it here --- it certainly wouldn't improve Stephen's original problem, much less the other issues he raises here. What would people think of just eliminating the access-permissions checks involved in temp_tablespaces? It would likely be appropriate to change temp_tablespaces from USERSET to SUSET if we did so. So essentially the worldview would become that the DBA is responsible for the temp_tablespaces setting, not individual users. 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] Reducing size of WAL record headers
Overall, the WAL record is MAXALIGN'd, so with 8 byte alignment we waste 4 bytes per record. Or put another way, if we could reduce record header by 4 bytes, we would actually reduce it by 8 bytes per record. So looking for ways to do that seems like a good idea. The WAL record header starts with xl_tot_len, a 4 byte field. There is also another field, xl_len. The difference is that xl_tot_len includes the header, xl_len and any backup blocks. Since the header is fixed, the only time xl_tot_len != SizeOfXLogRecord + xl_len is when we have backup blocks. We can re-arrange the record layout so that we remove xl_tot_len and add another (maxaligned) 4 byte field (-- 8 bytes) after the record header, xl_bkpblock_len that only exists if we have backup blocks. This will then save 8 bytes from every record that doesn't have backup blocks, and be the same as now with backup blocks. The only problem is that we currently allow WAL records to be written so that the header wraps across pages. This allows us to save space in WAL when we have between 5 and 32 bytes spare at the end of a page. To reduce the header size by 8 bytes we would need to ensure that the whole header, which would now be 24 or 32 bytes, is all on one page. My math tells me that would waste on average 12 bytes per page because of the end-of-page wastage, but would gain 8 bytes per record when we don't have backup blocks. My thinking is that the end of page loss would be much reduced on average when we had backup blocks, so we could ignore that case. Assuming typically 100 records per page when we have no backup blocks, this is a considerable upside. We would make gains on any page with 3 or more WAL records on it, so low downside even in worst cases. That seems like a great break-even point for optimisation. Since we've changed the WAL format already this release, another change seems OK. More to the point, we can remove backup blocks in the common case without changing WAL format, so this might be the last time we have the chance to make this change. Forcing the XLogRecord header to be all on one page makes the format more robust and simplifies the code that copes with header wrapping. The format changes would mean that its still possible to work out the length of the WAL record precisely = SizeOfXLogRecord + (HasBkpBlocks ? SizeOf(uint32) : 0) + xl_len and so would then be protected by the WAL record CRC. Thoughts? -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Feature Request: pg_replication_master()
On Thu, Jan 3, 2013 at 07:45:32PM +, Simon Riggs wrote: On 3 January 2013 18:35, Josh Berkus j...@agliodbs.com wrote: Robert, In my view, the biggest problem with recovery.conf is that the parameters in there are not GUCs, which means that all of the infrastructure that we've built for managing GUCs does not work with them. As an example, when we converted recovery.conf to use the same lexer that the GUC machinery uses, it allowed recovery.conf values to be specified unquoted in the same circumstances where that was already possible for postgresql.conf. But, you still can't use SHOW or pg_settings with recovery.conf parameters, and I think pg_ctl reload doesn't work either. If we make these parameters into GUCs, then they'll work the same way everything else works. Even if (as seems likely) we end up still needing a trigger file (or a special pg_ctl mode) to initiate recovery, I think that's probably a win. I agree that it would be an improvement, and I would be happy just to see the parameters become GUCs. That may be possible in 9.3 since we have a patch from Fujii-san. I'll hack that down to just the GUC part once we start the next CF. My personal priority is the shutdown checkpoint patch over that though. I'm just saying that I'll still be pushing to get rid of the requirement for recovery.conf in 9.4, that's all. No pushing required. When we have a reasonable proposal that improves on the current state, we can implement that. Sounds like we are all in agreement and on a good track to completion. I apologize for overreacting and thinking we were not addressing this issue objectively. Thanks for the discussion. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
I wrote:
I then applied the palloc.h and mcxt.c hunks of your patch and rebuilt.
Now I get an average runtime of 1 ms, a full 2% faster, which is a
bit astonishing, particularly because the oprofile results haven't moved
much:
I studied the assembly code being generated for palloc(), and I believe
I see the reason why it's a bit faster: when there's only a single local
variable that has to survive over the elog call, gcc generates a shorter
function entry/exit sequence. I had thought of proposing that we code
palloc() like this:
void *
palloc(Size size)
{
MemoryContext context = CurrentMemoryContext;
AssertArg(MemoryContextIsValid(context));
if (!AllocSizeIsValid(size))
elog(ERROR, invalid memory alloc request size %lu,
(unsigned long) size);
context-isReset = false;
return (*context-methods-alloc) (context, size);
}
but at least on this specific hardware and compiler that would evidently
be a net loss compared to direct use of CurrentMemoryContext. I would
not put a lot of faith in that result holding up on other machines
though.
In any case this doesn't explain the whole 2% speedup, but it probably
accounts for palloc() showing as slightly cheaper than
MemoryContextAlloc had been in the oprofile listing.
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] Index build temp files
* Tom Lane (t...@sss.pgh.pa.us) wrote: We do have mechanism that forces search_path to be recomputed across changes of active role, but it's expensive to do that, and it seems of rather debatable value to do it here --- it certainly wouldn't improve Stephen's original problem, much less the other issues he raises here. I agree that we would need something more for users to be able to be able to tell what temp tablespace their temporary tables will end up in. Generally, I think it'd be acceptable from a performance perspective to figure out where to create the temporary objects at the time that they are requested- at that point, you're going to be creating a new table, index, or doing a large sort that spills to disk, all of which are relatively heavy-weight operations. What would people think of just eliminating the access-permissions checks involved in temp_tablespaces? It would likely be appropriate to change temp_tablespaces from USERSET to SUSET if we did so. So essentially the worldview would become that the DBA is responsible for the temp_tablespaces setting, not individual users. I believe it's important to be able to control what temp tablespaces are used on a per-user basis and that 'set role;' or security definer functions respect the tablespace which has been set for the current role. Temp tablespaces are extremely valuable for managing users who unintentionally write run-away queries that fill up the tablespace. Keeping those seperate from the temp tablespace used for SECURITY DEFINER functions (which are written with care) or other ongoing system activity is necessary. Perhaps there would be a way to still do that with your approach, but it didn't sound like it initially. Perhaps we can simply keep track of what the current role was when we cache'd which temp tablespace was selected for a given session and, if the current role has changed, reconsider the temp tablespace selected? We would need to update the documentation to reflect that you might not *always* have the same tablespace for a session, if there are security definer and/or set role's happening, but that seems reasonable to me. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Index build temp files
* Simon Riggs (si...@2ndquadrant.com) wrote: I think we need to allow a TEMP permission on tablespaces, so that you aren't allowed to create normal objects but you can create TEMP objects and sort files there. I agree that this would be valuable. Would we end up needing to burn another bit off the aclmask though? That's certainly been a concern in the past and I don't recall that we ever improved that situation. I think SHOW temp_tablespaces should only show the valid tablespaces, not all of the ones actually listed in the parameter, otherwise you have no clue that the parameter setting is ineffectual for you. I like this as well. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Reducing size of WAL record headers
On 09.01.2013 22:36, Simon Riggs wrote: Overall, the WAL record is MAXALIGN'd, so with 8 byte alignment we waste 4 bytes per record. Or put another way, if we could reduce record header by 4 bytes, we would actually reduce it by 8 bytes per record. So looking for ways to do that seems like a good idea. Agreed. The WAL record header starts with xl_tot_len, a 4 byte field. There is also another field, xl_len. The difference is that xl_tot_len includes the header, xl_len and any backup blocks. Since the header is fixed, the only time xl_tot_len != SizeOfXLogRecord + xl_len is when we have backup blocks. We can re-arrange the record layout so that we remove xl_tot_len and add another (maxaligned) 4 byte field (-- 8 bytes) after the record header, xl_bkpblock_len that only exists if we have backup blocks. This will then save 8 bytes from every record that doesn't have backup blocks, and be the same as now with backup blocks. Here's a better idea: Let's keep xl_tot_len as it is, but move xl_len at the very end of the WAL record, after all the backup blocks. If there are no backup blocks, xl_len is omitted. Seems more robust to keep xl_tot_len, so that you require less math to figure out where one record ends and where the next one begins. Forcing the XLogRecord header to be all on one page makes the format more robust and simplifies the code that copes with header wrapping. -1 on that. That would essentially revert the changes I made earlier. The purpose of allowing the header to be wrapped was that you could easily calculate ahead of time exactly how much space a WAL record takes. My motivation for that was the XLogInsert scaling patch. Now, I admit I haven't had a chance to work further on that patch, so we're not gaining much from the format change at the moment. Nevertheless, I don't want us to get back to the situation that you sometimes need to add padding to the end of a WAL page. My suggestion above to keep xl_tot_len and remove xl_len from XLogRecord doesn't have a problem with crossing page boundaries. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reducing size of WAL record headers
On Wed, Jan 9, 2013 at 10:54:33PM +0200, Heikki Linnakangas wrote: On 09.01.2013 22:36, Simon Riggs wrote: Overall, the WAL record is MAXALIGN'd, so with 8 byte alignment we waste 4 bytes per record. Or put another way, if we could reduce record header by 4 bytes, we would actually reduce it by 8 bytes per record. So looking for ways to do that seems like a good idea. Agreed. The WAL record header starts with xl_tot_len, a 4 byte field. There is also another field, xl_len. The difference is that xl_tot_len includes the header, xl_len and any backup blocks. Since the header is fixed, the only time xl_tot_len != SizeOfXLogRecord + xl_len is when we have backup blocks. We can re-arrange the record layout so that we remove xl_tot_len and add another (maxaligned) 4 byte field (-- 8 bytes) after the record header, xl_bkpblock_len that only exists if we have backup blocks. This will then save 8 bytes from every record that doesn't have backup blocks, and be the same as now with backup blocks. Here's a better idea: Let's keep xl_tot_len as it is, but move xl_len at the very end of the WAL record, after all the backup blocks. If there are no backup blocks, xl_len is omitted. Seems more robust to keep xl_tot_len, so that you require less math to figure out where one record ends and where the next one begins. OK, crazy idea, but can we just record xl_len as a difference against xl_tot_len, and shorten the xl_len field? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reducing size of WAL record headers
On 09.01.2013 22:59, Bruce Momjian wrote: On Wed, Jan 9, 2013 at 10:54:33PM +0200, Heikki Linnakangas wrote: On 09.01.2013 22:36, Simon Riggs wrote: The WAL record header starts with xl_tot_len, a 4 byte field. There is also another field, xl_len. The difference is that xl_tot_len includes the header, xl_len and any backup blocks. Since the header is fixed, the only time xl_tot_len != SizeOfXLogRecord + xl_len is when we have backup blocks. We can re-arrange the record layout so that we remove xl_tot_len and add another (maxaligned) 4 byte field (-- 8 bytes) after the record header, xl_bkpblock_len that only exists if we have backup blocks. This will then save 8 bytes from every record that doesn't have backup blocks, and be the same as now with backup blocks. Here's a better idea: Let's keep xl_tot_len as it is, but move xl_len at the very end of the WAL record, after all the backup blocks. If there are no backup blocks, xl_len is omitted. Seems more robust to keep xl_tot_len, so that you require less math to figure out where one record ends and where the next one begins. OK, crazy idea, but can we just record xl_len as a difference against xl_tot_len, and shorten the xl_len field? Hmm, so it would essentially be the length of all the backup blocks. perhaps rename it to xl_bkpblk_len. However, that would cap the total size of backup blocks to 64k. Which would not be enough with 32k BLCKSZ. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Index build temp files
Stephen Frost sfr...@snowman.net writes: * Tom Lane (t...@sss.pgh.pa.us) wrote: We do have mechanism that forces search_path to be recomputed across changes of active role, but it's expensive to do that, and it seems of rather debatable value to do it here --- it certainly wouldn't improve Stephen's original problem, much less the other issues he raises here. Generally, I think it'd be acceptable from a performance perspective to figure out where to create the temporary objects at the time that they are requested- at that point, you're going to be creating a new table, index, or doing a large sort that spills to disk, all of which are relatively heavy-weight operations. It's not so much performance I'm worried about here as modularity/layering issues. Actual use of the temp-tablespaces list to create temp files is in fd.c, which has no business invoking catalog accesses, so we can't just say we'll recheck the permissions when we're going to create some temp files. In any case this wouldn't improve the behavior you were originally on about, which was surprising (to you) failure to use a DBA-specified temp_tablespaces list. What would people think of just eliminating the access-permissions checks involved in temp_tablespaces? It would likely be appropriate to change temp_tablespaces from USERSET to SUSET if we did so. So essentially the worldview would become that the DBA is responsible for the temp_tablespaces setting, not individual users. I believe it's important to be able to control what temp tablespaces are used on a per-user basis and that 'set role;' or security definer functions respect the tablespace which has been set for the current role. Temp tablespaces are extremely valuable for managing users who unintentionally write run-away queries that fill up the tablespace. IIRC, we do have mechanism now whereby a superuser can establish settings for SUSET variables via ALTER ROLE SET/ALTER DATABASE SET, and everything works as expected. So the only loss of flexibility here would be if you want unprivileged code to be able to set temp_tablespaces for itself. However, if you want that then it's hard to argue that there shouldn't be a permissions check, and then we're back into the surprises mentioned previously. It might be possible to compromise on an arrangement whereby tablespace permissions checking only occurs if the active value of the variable was set by a non-superuser. But TBH that idea fills me with dread --- we don't have any other GUCs that work like that, and it seems too likely that there would be conceptual or implementation bugs in 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] PL/perl should fail on configure, not make
On 01/09/2013 10:16 AM, Tom Lane wrote:
Andrew Dunstan and...@dunslane.net writes:
On 01/08/2013 10:37 PM, Tom Lane wrote:
We could try adding an AC_TRY_LINK test using perl_embed_ldflags,
but given the weird stuff happening to redefine that value on Windows
in plperl/GNUmakefile I think there's a serious risk of breaking Cygwin
builds. Since I lack access to either Cygwin or a platform on which
there's a problem today, I'm not going to be the one to mess with it.
ITYM Mingw - the Makefile doesn't do anything for Cygwin.
OK, sorry.
If you want to build a configure test, you could make it conditional on
the PORTNAME not being win32, since we don't seem to have a problem
there anyway.
Actually, if we were to try to clean this up, I'd suggest moving that
logic into the configure script --- it's not apparent to me why it's
a good idea to be changing configure-determined values in the Makefile.
But in any case this would have to be done by somebody who's in a
position to test on affected platforms.
Here's a patch which does that and produces configure traces like this
on Mingw:
checking for Perl archlibexp... C:/Perl/lib
checking for Perl privlibexp... C:/Perl/lib
checking for Perl useshrplib... true
checking for flags to link embedded Perl... -LC:/Perl/lib/CORE -lperl512
which seems to be what we want.
Given that, you should be able to write a reasonably portable configure
test for library presence.
Barring objection I'll apply this shortly.
cheers
andrew
diff --git a/config/perl.m4 b/config/perl.m4
index d602a5b..0b43b04 100644
--- a/config/perl.m4
+++ b/config/perl.m4
@@ -38,6 +38,7 @@ AC_DEFUN([PGAC_CHECK_PERL_CONFIG],
[AC_REQUIRE([PGAC_PATH_PERL])
AC_MSG_CHECKING([for Perl $1])
perl_$1=`$PERL -MConfig -e 'print $Config{$1}'`
+test $PORTNAME = win32 perl_$1=`echo $perl_$1 | sed 's,,/,g'`
AC_SUBST(perl_$1)dnl
AC_MSG_RESULT([$perl_$1])])
@@ -57,9 +58,14 @@ AC_DEFUN([PGAC_CHECK_PERL_CONFIGS],
AC_DEFUN([PGAC_CHECK_PERL_EMBED_LDFLAGS],
[AC_REQUIRE([PGAC_PATH_PERL])
AC_MSG_CHECKING(for flags to link embedded Perl)
+if test $PORTNAME = win32 ; then
+perl_lib=`basename $perl_archlibexp/CORE/perl[[5-9]]*.lib .lib`
+test -e $perl_archlibexp/CORE/$perl_lib.lib perl_embed_ldflags=-L$perl_archlibexp/CORE -l$perl_lib
+else
pgac_tmp1=`$PERL -MExtUtils::Embed -e ldopts`
pgac_tmp2=`$PERL -MConfig -e 'print $Config{ccdlflags}'`
perl_embed_ldflags=`echo X$pgac_tmp1 | sed -e s/^X// -e s%$pgac_tmp2%% -e [s/ -arch [-a-zA-Z0-9_]*//g]`
+fi
AC_SUBST(perl_embed_ldflags)dnl
if test -z $perl_embed_ldflags ; then
AC_MSG_RESULT(no)
diff --git a/configure b/configure
index c6ffe6c..e7f70ee 100755
--- a/configure
+++ b/configure
@@ -7320,24 +7320,32 @@ $as_echo $as_me: error: Perl not found 2;}
{ $as_echo $as_me:$LINENO: checking for Perl archlibexp 5
$as_echo_n checking for Perl archlibexp... 6; }
perl_archlibexp=`$PERL -MConfig -e 'print $Config{archlibexp}'`
+test $PORTNAME = win32 perl_archlibexp=`echo $perl_archlibexp | sed 's,,/,g'`
{ $as_echo $as_me:$LINENO: result: $perl_archlibexp 5
$as_echo $perl_archlibexp 6; }
{ $as_echo $as_me:$LINENO: checking for Perl privlibexp 5
$as_echo_n checking for Perl privlibexp... 6; }
perl_privlibexp=`$PERL -MConfig -e 'print $Config{privlibexp}'`
+test $PORTNAME = win32 perl_privlibexp=`echo $perl_privlibexp | sed 's,,/,g'`
{ $as_echo $as_me:$LINENO: result: $perl_privlibexp 5
$as_echo $perl_privlibexp 6; }
{ $as_echo $as_me:$LINENO: checking for Perl useshrplib 5
$as_echo_n checking for Perl useshrplib... 6; }
perl_useshrplib=`$PERL -MConfig -e 'print $Config{useshrplib}'`
+test $PORTNAME = win32 perl_useshrplib=`echo $perl_useshrplib | sed 's,,/,g'`
{ $as_echo $as_me:$LINENO: result: $perl_useshrplib 5
$as_echo $perl_useshrplib 6; }
{ $as_echo $as_me:$LINENO: checking for flags to link embedded Perl 5
$as_echo_n checking for flags to link embedded Perl... 6; }
+if test $PORTNAME = win32 ; then
+perl_lib=`basename $perl_archlibexp/CORE/perl[5-9]*.lib .lib`
+test -e $perl_archlibexp/CORE/$perl_lib.lib perl_embed_ldflags=-L$perl_archlibexp/CORE -l$perl_lib
+else
pgac_tmp1=`$PERL -MExtUtils::Embed -e ldopts`
pgac_tmp2=`$PERL -MConfig -e 'print $Config{ccdlflags}'`
perl_embed_ldflags=`echo X$pgac_tmp1 | sed -e s/^X// -e s%$pgac_tmp2%% -e s/ -arch [-a-zA-Z0-9_]*//g`
+fi
if test -z $perl_embed_ldflags ; then
{ $as_echo $as_me:$LINENO: result: no 5
$as_echo no 6; }
diff --git a/src/pl/plperl/GNUmakefile b/src/pl/plperl/GNUmakefile
index e1f9493..e0e31ec 100644
--- a/src/pl/plperl/GNUmakefile
+++ b/src/pl/plperl/GNUmakefile
@@ -16,10 +16,6 @@ endif
ifeq ($(shared_libperl),yes)
ifeq ($(PORTNAME), win32)
-perl_archlibexp := $(subst \,/,$(perl_archlibexp))
-perl_privlibexp := $(subst \,/,$(perl_privlibexp))
-perl_lib := $(basename $(notdir $(wildcard $(perl_archlibexp)/CORE/perl[5-9]*.lib)))
-perl_embed_ldflags = -L$(perl_archlibexp)/CORE -l$(perl_lib)
override CPPFLAGS += -DPLPERL_HAVE_UID_GID
#
Re: [HACKERS] Index build temp files
On 9 January 2013 20:53, Stephen Frost sfr...@snowman.net wrote: * Simon Riggs (si...@2ndquadrant.com) wrote: I think we need to allow a TEMP permission on tablespaces, so that you aren't allowed to create normal objects but you can create TEMP objects and sort files there. I agree that this would be valuable. Would we end up needing to burn another bit off the aclmask though? That's certainly been a concern in the past and I don't recall that we ever improved that situation. There already is a TEMP permission, it just only applies to databases at present. I think SHOW temp_tablespaces should only show the valid tablespaces, not all of the ones actually listed in the parameter, otherwise you have no clue that the parameter setting is ineffectual for you. I like this as well. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PL/perl should fail on configure, not make
Andrew Dunstan and...@dunslane.net writes: On 01/09/2013 10:16 AM, Tom Lane wrote: Actually, if we were to try to clean this up, I'd suggest moving that logic into the configure script --- it's not apparent to me why it's a good idea to be changing configure-determined values in the Makefile. But in any case this would have to be done by somebody who's in a position to test on affected platforms. Here's a patch which does that and produces configure traces like this on Mingw: checking for Perl archlibexp... C:/Perl/lib checking for Perl privlibexp... C:/Perl/lib checking for Perl useshrplib... true checking for flags to link embedded Perl... -LC:/Perl/lib/CORE -lperl512 which seems to be what we want. Given that, you should be able to write a reasonably portable configure test for library presence. Looks good. If you're happy with that then I can undertake to add a libperl.so probe based on AC_TRY_LINK with the unmodified value of $perl_embed_ldflags. 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] Reducing size of WAL record headers
On 9 January 2013 21:02, Heikki Linnakangas hlinnakan...@vmware.com wrote: OK, crazy idea, but can we just record xl_len as a difference against xl_tot_len, and shorten the xl_len field? Hmm, so it would essentially be the length of all the backup blocks. perhaps rename it to xl_bkpblk_len. However, that would cap the total size of backup blocks to 64k. Which would not be enough with 32k BLCKSZ. Since that requires a recompile anyway, why not make XLogRecord smaller only for 16k BLCKSZ or less? Problem if we do that is that xl_len is used extensively in _redo routines, so its a much more invasive patch. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reducing size of WAL record headers
On 9 January 2013 20:54, Heikki Linnakangas hlinnakan...@vmware.com wrote: Here's a better idea: Let's keep xl_tot_len as it is, but move xl_len at the very end of the WAL record, after all the backup blocks. If there are no backup blocks, xl_len is omitted. Seems more robust to keep xl_tot_len, so that you require less math to figure out where one record ends and where the next one begins. OK, I avoided tampering with xl_len cos its so widely used. Will look. Forcing the XLogRecord header to be all on one page makes the format more robust and simplifies the code that copes with header wrapping. -1 on that. That would essentially revert the changes I made earlier. OK, idea dropped. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Index build temp files
Simon Riggs si...@2ndquadrant.com writes: On 9 January 2013 20:53, Stephen Frost sfr...@snowman.net wrote: * Simon Riggs (si...@2ndquadrant.com) wrote: I think we need to allow a TEMP permission on tablespaces, so that you aren't allowed to create normal objects but you can create TEMP objects and sort files there. I agree that this would be valuable. Would we end up needing to burn another bit off the aclmask though? That's certainly been a concern in the past and I don't recall that we ever improved that situation. There already is a TEMP permission, it just only applies to databases at present. This sounds like rather a lot of work to create a behavior that doesn't solve the originally-complained-of usability problem. All it does is make things even more complicated, and add an extra step for the DBA who's just trying to set temp_tablespaces to something useful. (Or are you proposing that we'd grant TEMP permission to PUBLIC by default? We'd have compatibility issues with how to interpret the grants in existing dump files if we do that.) 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] Index build temp files
* Tom Lane (t...@sss.pgh.pa.us) wrote: IIRC, we do have mechanism now whereby a superuser can establish settings for SUSET variables via ALTER ROLE SET/ALTER DATABASE SET, and everything works as expected. I'm not entirely convinced that it works as expected, at least for temp_tablespaces currently. This was a bit surprising to me: =# alter user test_role set temp_tablespaces='zz'; NOTICE: tablespace zz does not exist ALTER ROLE =*# commit; COMMIT =# set role test_role; SET =* show temp_tablespaces; temp_tablespaces -- rn_temp_01 (1 row) So the only loss of flexibility here would be if you want unprivileged code to be able to set temp_tablespaces for itself. I'm on the fence about this one. I could see that being useful in some cases when there are a lot of temporary tables being created and you need more than one tablespace for simple space reasons. It'd be unfortuante if you had to be a superuser and/or call a superuser security definer function to handle that situation. However, if you want that then it's hard to argue that there shouldn't be a permissions check, and then we're back into the surprises mentioned previously. I'm not sure we're going to be able to get away from that. It might be possible to compromise on an arrangement whereby tablespace permissions checking only occurs if the active value of the variable was set by a non-superuser. But TBH that idea fills me with dread --- we don't have any other GUCs that work like that, and it seems too likely that there would be conceptual or implementation bugs in it. I don't particularly like that either. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Index build temp files
On 9 January 2013 21:21, Tom Lane t...@sss.pgh.pa.us wrote: Simon Riggs si...@2ndquadrant.com writes: On 9 January 2013 20:53, Stephen Frost sfr...@snowman.net wrote: * Simon Riggs (si...@2ndquadrant.com) wrote: I think we need to allow a TEMP permission on tablespaces, so that you aren't allowed to create normal objects but you can create TEMP objects and sort files there. I agree that this would be valuable. Would we end up needing to burn another bit off the aclmask though? That's certainly been a concern in the past and I don't recall that we ever improved that situation. There already is a TEMP permission, it just only applies to databases at present. This sounds like rather a lot of work to create a behavior that doesn't solve the originally-complained-of usability problem. All it does is make things even more complicated, and add an extra step for the DBA who's just trying to set temp_tablespaces to something useful. There is already an extra step to GRANT the CREATE privilege, so how does this change things? The whole point of the thread is that that step was omitted. Oracle (and IIRC others) allow tablespaces to be created as TEMP, so that nobody can put normal objects in there. That is a useful facility and there's no way currently in Postgres of setting things up like that. Hence the proposal. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Index build temp files
Simon Riggs si...@2ndquadrant.com writes: On 9 January 2013 21:21, Tom Lane t...@sss.pgh.pa.us wrote: This sounds like rather a lot of work to create a behavior that doesn't solve the originally-complained-of usability problem. All it does is make things even more complicated, and add an extra step for the DBA who's just trying to set temp_tablespaces to something useful. There is already an extra step to GRANT the CREATE privilege, so how does this change things? The point is that it didn't occur to Stephen that putting temp_tablespaces = 'foo, bar' into postgresql.conf should require doing GRANT CREATE TO PUBLIC on those tablespaces in order to be effective. Changing the situation so that instead he needs to do GRANT TEMP TO PUBLIC does not make it one whit more usable. All that that will really accomplish is to break grant methods that are working in (other people's) existing installations; ie if someone has code that does know about the GRANT CREATE requirement, he will not thank us if he suddenly has to spell it GRANT TEMP in the next release. If we were designing this from scratch I'd agree that a separate TEMP privilege would be a good thing. But bolting one on now is likely to create more problems than it fixes. Particularly since it doesn't actually fix any of the concrete problems enumerated in this thread. I continue to think that getting rid of the privilege check would be a more useful answer than changing which privilege is tested. 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] Reducing size of WAL record headers
On Wed, Jan 9, 2013 at 09:15:16PM +, Simon Riggs wrote: On 9 January 2013 21:02, Heikki Linnakangas hlinnakan...@vmware.com wrote: OK, crazy idea, but can we just record xl_len as a difference against xl_tot_len, and shorten the xl_len field? Hmm, so it would essentially be the length of all the backup blocks. perhaps rename it to xl_bkpblk_len. However, that would cap the total size of backup blocks to 64k. Which would not be enough with 32k BLCKSZ. Since that requires a recompile anyway, why not make XLogRecord smaller only for 16k BLCKSZ or less? Problem if we do that is that xl_len is used extensively in _redo routines, so its a much more invasive patch. I would just make it int16 on =16k block size, and int32 on 16k blocks. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PL/perl should fail on configure, not make
On 01/09/2013 04:12 PM, Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: On 01/09/2013 10:16 AM, Tom Lane wrote: Actually, if we were to try to clean this up, I'd suggest moving that logic into the configure script --- it's not apparent to me why it's a good idea to be changing configure-determined values in the Makefile. But in any case this would have to be done by somebody who's in a position to test on affected platforms. Here's a patch which does that and produces configure traces like this on Mingw: checking for Perl archlibexp... C:/Perl/lib checking for Perl privlibexp... C:/Perl/lib checking for Perl useshrplib... true checking for flags to link embedded Perl... -LC:/Perl/lib/CORE -lperl512 which seems to be what we want. Given that, you should be able to write a reasonably portable configure test for library presence. Looks good. If you're happy with that then I can undertake to add a libperl.so probe based on AC_TRY_LINK with the unmodified value of $perl_embed_ldflags. Go for it. 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] Index build temp files
On 9 January 2013 21:42, Tom Lane t...@sss.pgh.pa.us wrote: Simon Riggs si...@2ndquadrant.com writes: On 9 January 2013 21:21, Tom Lane t...@sss.pgh.pa.us wrote: This sounds like rather a lot of work to create a behavior that doesn't solve the originally-complained-of usability problem. All it does is make things even more complicated, and add an extra step for the DBA who's just trying to set temp_tablespaces to something useful. There is already an extra step to GRANT the CREATE privilege, so how does this change things? The point is that it didn't occur to Stephen that putting temp_tablespaces = 'foo, bar' into postgresql.conf should require doing GRANT CREATE TO PUBLIC on those tablespaces in order to be effective. Changing the situation so that instead he needs to do GRANT TEMP TO PUBLIC does not make it one whit more usable. All that that will really accomplish is to break grant methods that are working in (other people's) existing installations; ie if someone has code that does know about the GRANT CREATE requirement, he will not thank us if he suddenly has to spell it GRANT TEMP in the next release. If we were designing this from scratch I'd agree that a separate TEMP privilege would be a good thing. But bolting one on now is likely to create more problems than it fixes. Particularly since it doesn't actually fix any of the concrete problems enumerated in this thread. I continue to think that getting rid of the privilege check would be a more useful answer than changing which privilege is tested. I wasn't suggesting that we test for TEMP instead of CREATE; what I meant was we would test for CREATE *OR* TEMP to give more options for management. Since CREATE is a powerful privilege, secure systems would not wish to grant that to everyone, which is what I think caused the issue, coupled with the inability to know whether temp_tablespaces is set to something you have privileges on. Your suggestion to make TEMP the default would be a useful way to handle this, but its still the opposite of how things work now. Granting CREATE by default on tablespaces is not a great plan. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reducing size of WAL record headers
Simon Riggs si...@2ndquadrant.com writes: Overall, the WAL record is MAXALIGN'd, so with 8 byte alignment we waste 4 bytes per record. Or put another way, if we could reduce record header by 4 bytes, we would actually reduce it by 8 bytes per record. So looking for ways to do that seems like a good idea. I think this is extremely premature, in view of the ongoing discussions about shoehorning logical replication and other kinds of data into the WAL stream. It seems quite likely that we'll end up eating some of that padding space to support those features. So whacking a lot of code around in service of squeezing the existing padding out could very easily end up being wasted work, in fact counterproductive if it degrades either code readability or robustness. Let's wait till we see where the logical rep stuff ends up before we worry about saving 4 bytes per WAL record. 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] Index build temp files
Simon Riggs si...@2ndquadrant.com writes: On 9 January 2013 21:42, Tom Lane t...@sss.pgh.pa.us wrote: If we were designing this from scratch I'd agree that a separate TEMP privilege would be a good thing. But bolting one on now is likely to create more problems than it fixes. Particularly since it doesn't actually fix any of the concrete problems enumerated in this thread. I continue to think that getting rid of the privilege check would be a more useful answer than changing which privilege is tested. I wasn't suggesting that we test for TEMP instead of CREATE; what I meant was we would test for CREATE *OR* TEMP to give more options for management. [ shrug... ] That's weird, ie unlike the behavior of other privileges, and it *still* doesn't fix any of the problems Stephen complained of. 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] Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
On 2013-01-09 15:43:19 -0500, Tom Lane wrote:
I wrote:
I then applied the palloc.h and mcxt.c hunks of your patch and rebuilt.
Now I get an average runtime of 1 ms, a full 2% faster, which is a
bit astonishing, particularly because the oprofile results haven't moved
much:
I studied the assembly code being generated for palloc(), and I believe
I see the reason why it's a bit faster: when there's only a single local
variable that has to survive over the elog call, gcc generates a shorter
function entry/exit sequence.
Makes sense.
I had thought of proposing that we code
palloc() like this:
void *
palloc(Size size)
{
MemoryContext context = CurrentMemoryContext;
AssertArg(MemoryContextIsValid(context));
if (!AllocSizeIsValid(size))
elog(ERROR, invalid memory alloc request size %lu,
(unsigned long) size);
context-isReset = false;
return (*context-methods-alloc) (context, size);
}
but at least on this specific hardware and compiler that would evidently
be a net loss compared to direct use of CurrentMemoryContext. I would
not put a lot of faith in that result holding up on other machines
though.
Thats not optimized to the same? ISTM the compiler should produce
exactly the same code for both.
In any case this doesn't explain the whole 2% speedup, but it probably
accounts for palloc() showing as slightly cheaper than
MemoryContextAlloc had been in the oprofile listing.
I'd guess that a good part of the cost is just smeared across all
callers and not individually accountable to any function visible in the
profile. Additionally, With functions as short as MemoryContextAllocZero
profiles like oprofile (and perf) also often leak quite a bit of the
actual cost to the callsites in my experience.
I wonder whether it makes sense to inline the contents pstrdup()
additionally? My gut feeling is not, but...
I would like to move CurrentMemoryContext to memutils.h, but that seems
to require too many changes.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
On 2013-01-09 15:07:10 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: Well, I *did* benchmark it as noted elsewhere in the thread, but thats obviously just machine (E5520 x 2) with one rather restricted workload (pgbench -S -jc 40 -T60). At least its rather palloc heavy. Here are the numbers: before: #101646.763208 101350.361595 101421.425668 101571.211688 101862.172051 101449.857665 after: #101553.596257 102132.277795 101528.816229 101733.541792 101438.531618 101673.400992 So on my system if there is a difference, its positive (0.12%). pgbench-based testing doesn't fill me with a lot of confidence for this --- its numbers contain a lot of communication overhead, not to mention that pgbench itself can be a bottleneck. I chose it because I looked at profiles of it often enough to know memory allocation shows up high in the profile (especially without -M prepared). I would like to know if other people get comparable results on other hardware (non-Intel hardware would be especially interesting). If this result holds up across a range of platforms, I'll withdraw my objection to making palloc a plain function. I only have access to core2 and Nehalem atm, but FWIW I just tested it on another workload (decoding WAL changes of a large transaction into text) and I see improvements around 1.2% on both. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Andres Freund and...@2ndquadrant.com writes:
On 2013-01-09 15:43:19 -0500, Tom Lane wrote:
I had thought of proposing that we code
palloc() like this:
void *
palloc(Size size)
{
MemoryContext context = CurrentMemoryContext;
AssertArg(MemoryContextIsValid(context));
if (!AllocSizeIsValid(size))
elog(ERROR, invalid memory alloc request size %lu,
(unsigned long) size);
context-isReset = false;
return (*context-methods-alloc) (context, size);
}
but at least on this specific hardware and compiler that would evidently
be a net loss compared to direct use of CurrentMemoryContext. I would
not put a lot of faith in that result holding up on other machines
though.
Thats not optimized to the same? ISTM the compiler should produce
exactly the same code for both.
No, that's exactly the point here, you can't just assume that you get
the same code; this is tense enough that a few instructions matter.
Remember we're considering non-assert builds, so the AssertArg vanishes.
So in the form where CurrentMemoryContext is only read after the elog
call, the compiler can see that it requires but one fetch - it can't
change within the two lines where it's used. In the form given above,
the compiler is required to fetch CurrentMemoryContext into a local
variable and keep it across the elog call. (We know this doesn't
matter, but gcc doesn't; this version of gcc apparently isn't doing much
with the knowledge that elog won't return.) Since the extra local
variable adds several instructions to the overall function's entry/exit
sequences, you come out behind. At least on this platform. On other
machines with different code generation behavior, the story could easily
be very different.
In any case this doesn't explain the whole 2% speedup, but it probably
accounts for palloc() showing as slightly cheaper than
MemoryContextAlloc had been in the oprofile listing.
I'd guess that a good part of the cost is just smeared across all
callers and not individually accountable to any function visible in the
profile.
Yeah, I think this is most likely showing that there is a real,
measurable cost of code bloat.
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] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Andres Freund and...@2ndquadrant.com writes: I wonder whether it makes sense to inline the contents pstrdup() additionally? My gut feeling is not, but... I don't recall ever having seen MemoryContextStrdup amount to much, so probably not worth the extra code space. 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] Index build temp files
On 9 January 2013 22:09, Tom Lane t...@sss.pgh.pa.us wrote: Simon Riggs si...@2ndquadrant.com writes: On 9 January 2013 21:42, Tom Lane t...@sss.pgh.pa.us wrote: If we were designing this from scratch I'd agree that a separate TEMP privilege would be a good thing. But bolting one on now is likely to create more problems than it fixes. Particularly since it doesn't actually fix any of the concrete problems enumerated in this thread. I continue to think that getting rid of the privilege check would be a more useful answer than changing which privilege is tested. I wasn't suggesting that we test for TEMP instead of CREATE; what I meant was we would test for CREATE *OR* TEMP to give more options for management. [ shrug... ] That's weird, ie unlike the behavior of other privileges, and it *still* doesn't fix any of the problems Stephen complained of. I think we're having a disconnection half hour? The privs could be seen as CREATE_ANY or CREATE_TEMP_ONLY. One is a superset of the other, just like granting ANY is a superset of other privs. My view is it would fix the root cause of the problem, as explained. If you wanted to make TEMP active by default, we can do that. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
