Re: [HACKERS] pg_ctl idempotent option

[Ashutosh Bapat]

On Tue, Jan 29, 2013 at 10:49 AM, Josh Berkus  wrote:

>
> > OK, I had some time to think about this.  Basically, we have three
> > outcomes for pg_ctl start:
> >
> >   server not running and pg_ctl start success
> >   server start failed
> >   server already running
> >
> > Can't we just assign different return values to these cases, e.g. 0, 1,
> > 2?  We already print output telling the user what happened.
>
> Not sure if that would work.  Too many admin scripts only check for
> error output, and not what the error code was.
>
> FWIW, the Solaris/Opensolaris service scripts, as well as the RH service
> scripts (I think), already handle things this way.  If you say:
>
> svcadm enable postgresql
>
> ... and postgres is already up, it just returns 0.  So it's a common
> pattern.
>
> So, alternate suggestions to "idempotent":
>
> --isup
> --isrunning
> --ignorerunning
>
> However, I'm really beginnging to think that a switch isn't correct, and
> what we need to do is to have a different pg_ctl *command*, e.g.:
>
> pg_ctl -D . on
> or
> pg_ctl -D . enable
>
> I doubt if that would help much. We will need to coin a new command for
stop as well. A new pg_ctl command would confuse user as to what it should
be using "on" or "start" in a given scenario. I think switch is better.
Above switches won't look good with stop. We need some word/phrase which is
good for both start and stop commands.


> > I don't think I like --force because it isn't clear if we are forcing
> > the start to have done something, or forcing the server to be running.
>
> Agfeed.
>
>
> --
> 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
>



-- 
Best Wishes,
Ashutosh Bapat
EntepriseDB Corporation
The Enterprise Postgres Company

Re: [HACKERS] BUG #7493: Postmaster messages unreadable in a Windows console

[Alexander Law]

Hello,
Thanks for fixing bug #6510!
Please look at the following l10n bug:
http://www.postgresql.org/message-id/502a26f1.6010...@gmail.com
and the proposed patch.

Best regards,
Alexander

>From 1e2d5f712744d4731b665724703c0da4971ea41e Mon Sep 17 00:00:00 2001
From: Alexander Lakhin 
Date: Mon, 28 Jan 2013 08:19:34 +0400
Subject: Fix postmaster messages encoding

---
 src/backend/main/main.c |6 ++
 1 file changed, 6 insertions(+)
 mode change 100644 => 100755 src/backend/main/main.c

diff --git a/src/backend/main/main.c b/src/backend/main/main.c
index 1173bda..b79a483
--- a/src/backend/main/main.c
+++ b/src/backend/main/main.c
@@ -89,6 +89,12 @@ main(int argc, char *argv[])
 	pgwin32_install_crashdump_handler();
 #endif
 
+/*
+* Use the platform encoding until the process connects to a database
+* and sets the appropriate encoding.
+*/
+	SetDatabaseEncoding(GetPlatformEncoding());
+
 	/*
 	 * Set up locale information from environment.	Note that LC_CTYPE and
 	 * LC_COLLATE will be overridden later from pg_control if we are in an
-- 
1.7.10.4




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] psql \l to accept patterns

[Peter Eisentraut]

On Mon, 2013-01-07 at 07:14 -0500, Peter Eisentraut wrote:
> Here is a patch for psql's \l command to accept patterns, like \d
> commands do.  While at it, I also added an "S" option to show system
> objects and removed system objects from the default display.  This might
> be a bit controversial, but it's how it was decided some time ago that
> the \d commands should act.

Most people didn't like the "S" option, so here is a revised patch that
just adds the pattern support.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 4c87d8a..8d0095e 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1689,12 +1689,13 @@ Meta-Commands
 
 
   
-\l (or \list)
-\l+ (or \list+)
+\l[+] or \list[+]
 
 
-List the names, owners, character set encodings, and access privileges
-of all the databases in the server.
+List the databases in the server and show their names, owners,
+character set encodings, and access privileges.
+If pattern is specified,
+only databases whose names match the pattern are listed.
 If + is appended to the command name, database
 sizes, default tablespaces, and descriptions are also displayed.
 (Size information is only available for databases that the current
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 20c45e2..e40f8b2 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -804,10 +804,22 @@ static bool do_edit(const char *filename_arg, PQExpBuffer query_buf,
 	}
 
 	/* \l is list databases */
-	else if (strcmp(cmd, "l") == 0 || strcmp(cmd, "list") == 0)
-		success = listAllDbs(false);
-	else if (strcmp(cmd, "l+") == 0 || strcmp(cmd, "list+") == 0)
-		success = listAllDbs(true);
+	else if (strcmp(cmd, "l") == 0 || strcmp(cmd, "list") == 0 ||
+			 strcmp(cmd, "l+") == 0 || strcmp(cmd, "list+") == 0)
+	{
+		char	   *pattern;
+		bool		show_verbose;
+
+		pattern = psql_scan_slash_option(scan_state,
+		 OT_NORMAL, NULL, true);
+
+		show_verbose = strchr(cmd, '+') ? true : false;
+
+		success = listAllDbs(pattern, show_verbose);
+
+		if (pattern)
+			free(pattern);
+	}
 
 	/*
 	 * large object things
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 8064a3d..046513d 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -641,7 +641,7 @@ static bool describeOneTSConfig(const char *oid, const char *nspname,
  * for \l, \list, and -l switch
  */
 bool
-listAllDbs(bool verbose)
+listAllDbs(const char *pattern, bool verbose)
 {
 	PGresult   *res;
 	PQExpBufferData buf;
@@ -684,6 +684,11 @@ static bool describeOneTSConfig(const char *oid, const char *nspname,
 	if (verbose && pset.sversion >= 8)
 		appendPQExpBuffer(&buf,
 		   "  JOIN pg_catalog.pg_tablespace t on d.dattablespace = t.oid\n");
+
+	if (pattern)
+		processSQLNamePattern(pset.db, &buf, pattern, false, false,
+			  NULL, "d.datname", NULL, NULL);
+
 	appendPQExpBuffer(&buf, "ORDER BY 1;");
 	res = PSQLexec(buf.data, false);
 	termPQExpBuffer(&buf);
diff --git a/src/bin/psql/describe.h b/src/bin/psql/describe.h
index 9e71a88..09b6237 100644
--- a/src/bin/psql/describe.h
+++ b/src/bin/psql/describe.h
@@ -55,7 +55,7 @@ extern bool listTSDictionaries(const char *pattern, bool verbose);
 extern bool listTSTemplates(const char *pattern, bool verbose);
 
 /* \l */
-extern bool listAllDbs(bool verbose);
+extern bool listAllDbs(const char *pattern, bool verbose);
 
 /* \dt, \di, \ds, \dS, etc. */
 extern bool listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSystem);
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index fd7effa..2cbdd83 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -233,7 +233,7 @@
 	fprintf(output, _("  \\dE[S+] [PATTERN]  list foreign tables\n"));
 	fprintf(output, _("  \\dx[+]  [PATTERN]  list extensions\n"));
 	fprintf(output, _("  \\dy [PATTERN]  list event triggers\n"));
-	fprintf(output, _("  \\l[+]  list all databases\n"));
+	fprintf(output, _("  \\l[+]   [PATTERN]  list databases\n"));
 	fprintf(output, _("  \\sf[+] FUNCNAMEshow a function's definition\n"));
 	fprintf(output, _("  \\z  [PATTERN]  same as \\dp\n"));
 	fprintf(output, "\n");
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index a59f45b..5cb6b5f 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -260,7 +260,7 @@ static void parse_psql_options(int argc, char *argv[],
 		if (!options.no_psqlrc)
 			process_psqlrc(argv[0]);
 
-		success = listAllDbs(false);
+		success = listAllDbs(NULL, false);
 		PQfinish(pset.db);
 		exit(success ? EXIT_SUCCESS : EXIT_FAILURE);
 	}

-- 
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_ctl idempotent option

[Josh Berkus]

> OK, I had some time to think about this.  Basically, we have three
> outcomes for pg_ctl start:
> 
>   server not running and pg_ctl start success
>   server start failed
>   server already running
> 
> Can't we just assign different return values to these cases, e.g. 0, 1,
> 2?  We already print output telling the user what happened.

Not sure if that would work.  Too many admin scripts only check for
error output, and not what the error code was.

FWIW, the Solaris/Opensolaris service scripts, as well as the RH service
scripts (I think), already handle things this way.  If you say:

svcadm enable postgresql

... and postgres is already up, it just returns 0.  So it's a common
pattern.

So, alternate suggestions to "idempotent":

--isup
--isrunning
--ignorerunning

However, I'm really beginnging to think that a switch isn't correct, and
what we need to do is to have a different pg_ctl *command*, e.g.:

pg_ctl -D . on
or
pg_ctl -D . enable

> I don't think I like --force because it isn't clear if we are forcing
> the start to have done something, or forcing the server to be running.

Agfeed.


-- 
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] autovacuum not prioritising for-wraparound tables

[Josh Berkus]

> I have to admit, I fail to see why this is a good idea. There isn't much
> of an efficiency bonus in freezing early (due to hint bits) and vacuums
> over vacuum_freeze_table_age are considerably more expensive as they
> have to scan the whole heap instead of using the visibilitymap. And if
> you don't vacuum the whole heap you can't lower relfrozenxid. So
> changing freeze_min_age doesn't help at all to avoid anti-wraparound
> vacuums.
> 
> Am I missing something?

Yep.  First, you're confusing vacuum_freeze_table_age  and
vacuum_freeze_min_age.  Second, you're not doing any arithmatic.

Let's do this by example.  TableA is a large table which receives an
almost constant stream of individual row updates, inserts, and deletes.

DEFAULTS:

XID 1:  First rows in TableA are updated.
XID 200m: Anti-wraparound autovac of TableA.
 All XIDs older than XID 100m set to FROZENXID.
XID 300m: Anti-wraparound autovac of TableA
 All XIDs older than XID 200M set to FROZENXID.
XID 400m: Anti-wraparound autovac of TableA
 All XIDs older than XID 300M set to FROZENXID.
XID 500m: Anti-wraparound autovac of TableA
 All XIDs older than XID 400M set to FROZENXID.
XID 600m: Anti-wraparound autovac of TableA
 All XIDs older than XID 500M set to FROZENXID.


vacuum_freeze_min_age = 1m

XID 1:  First rows in TableA are updated.
XID 200m: Anti-wraparound autovac of TableA.
 All XIDs older than XID 199m set to FROZENXID.
XID 399m: Anti-wraparound autovac of TableA
 All XIDs older than XID 398M set to FROZENXID.
XID 598m: Anti-wraparound autovac of TableA
 All XIDs older than XID 597M set to FROZENXID.


vacuum_freeze_min_age = 1m, autovacuum_freeze_max_age = 500m

XID 1:  First rows in TableA are updated.
XID 500m: Anti-wraparound autovac of TableA.
 All XIDs older than XID 499m set to FROZENXID.

As you can see, the current default settings cause 80% more wraparound
autovacs per table than vacuum_freeze_min_age of 1m would, and almost
500% more than what I consider sane settings would.  Just so that we can
preserve XIDs which are 90m transactions old.

-- 
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] Hm, table constraints aren't so unique as all that

[David Rowley]

> Tom Lane Wrote:
> Peter Geoghegan  writes:
> > On 29 January 2013 00:25, Tom Lane  wrote:
> > I can see the case for fixing this, but I don't feel that it's
> > particularly important that constraints be uniquely identifiable from
> > the proposed new errdata fields.
> 
> I think that we'll soon be buried in gripes if they're not.  Pretty much
the
> whole point of this patch is to allow applications to get rid of ad-hoc,
it-
> usually-works coding techniques.  I'd argue that not checking the entire
> constraint identity is about as fragile as trying to "sed"
> the constraint name out of a potentially-localized error message.
> In both cases, it often works fine, until the application's context
changes.


+1 here too. I'm feel I'm quite close to the front of the queue of
application developers waiting on enhances error fields. I'd personally
rather I noticed my application was broken during an testing an upgrade to
9.3 than somewhere down the line. I can't imagine renaming a constraint to
upgrade to 9.3 is going to be a showstopper for anyone.

Perhaps the release notes can contain a query to allow users to check this
pre-upgrade.

Regards 
David Rowley

> 
>   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] enhanced error fields

[Tom Lane]

I wrote:
> Rather what we've got is that constraints are uniquely named among
> those associated with a table, or with a domain.  So the correct
> unique key for a table constraint is table schema + table name +
> constraint name, whereas for a domain constraint it's domain schema +
> domain name + constraint name.  The current patch provides sufficient
> information to uniquely identify a table constraint, but not so much
> domain constraints.  Should we fix that?  I think it'd be legitimate
> to re-use SCHEMA_NAME for domain schema, but we'd need a new nonstandard
> field DOMAIN_NAME (or maybe better DATATYPE_NAME) if we want to fix it.

I have hacked up the code (but not yet the documentation) to support
this, but I found out that there's at least one place where this
definition of the field semantics is a bit awkward.  The issue is that
this definition presupposes that we want to complain about a table or
a domain, never both, because we're overloading both the SCHEMA_NAME
and CONSTRAINT_NAME fields for both purposes.  This is annoying in
validateDomainConstraint(), where we know the domain constraint that
we're complaining about and also the table/column containing the bad
value.  We can't fill in both TABLE_NAME and DATATYPE_NAME because
they both want to set SCHEMA_NAME, and perhaps not to the same value.

Since the error report is about a domain constraint, I don't have a big
problem deciding that the domain has to win this tug-of-war.  And it
could easily be argued that if we had separate fields and filled in
both, an application could get confused about whether we meant a table
constraint or a domain constraint.  (As submitted, the patch would
definitely have made it look like we were complaining about a table
constraint.)  But it's still annoying that we can't represent all the
info that is in the human-readable message.

I'm not sure we should allow corner cases like this to drive the design;
but if anyone has an idea about a cleaner way, let's hear 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] Re: Doc patch making firm recommendation for setting the value of commit_delay

[Noah Misch]

On Mon, Jan 28, 2013 at 04:29:12AM +, Peter Geoghegan wrote:
> On 28 January 2013 03:34, Noah Misch  wrote:
> > On the EBS configuration with volatile fsync timings, the variability didn't
> > go away with 15s runs.  On systems with stable fsync times, 15s was no 
> > better
> > than 2s.  Absent some particular reason to believe 5s is better than 2s, I
> > would leave it alone.
> 
> I'm not recommending doing so because I thought you'd be likely to get
> better numbers on EBS; obviously the variability you saw there likely
> had a lot to do with the fact that the underlying physical machines
> have multiple tenants. It has just been my observation that more
> consistent figures can be obtained (on my laptop) by using a
> pg_test_fsync --secs-per-test of about 5. That being the case, why
> take the chance with 2 seconds?

I can't get too excited about it either way.

> It isn't as if people run
> pg_test_fsync everyday, or that they cannot set --secs-per-test to
> whatever they like themselves. On the other hand, the cost of setting
> it too low could be quite high now, because the absolute values (and
> not just how different wal_sync_methods compare) is now important.

True.  You'd actually want to run the tool with a short interval to select a
wal_sync_method, then test the chosen method for a longer period to get an
accurate reading for commit_delay.


-- 
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] Hm, table constraints aren't so unique as all that

[Robert Haas]

On Mon, Jan 28, 2013 at 10:23 PM, Tom Lane  wrote:
> I think that we'll soon be buried in gripes if they're not.  Pretty much
> the whole point of this patch is to allow applications to get rid of
> ad-hoc, it-usually-works coding techniques.  I'd argue that not checking
> the entire constraint identity is about as fragile as trying to "sed"
> the constraint name out of a potentially-localized error message.
> In both cases, it often works fine, until the application's context
> changes.

+1.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] autovacuum not prioritising for-wraparound tables

[Robert Haas]

On Mon, Jan 28, 2013 at 8:11 AM, Josh Berkus  wrote:
>> So I think we need to sort by age(relfrozenxid) in tables that are over
>> the anti-wraparound limit. Given your code that doesn't seem to be that
>> hard?
>
> I might also suggest that we think about changing the defaults for
> wraparound vacuum behavior.  Partcularly, the fact that
> vacuum_freeze_min_age is 50% of autovacuum_freeze_max_age by default is
> optimal for absolutely nobody, and forces re-wraparound vacuuming of
> wraparound tables which were just recently wraparound-vacuumed.  We
> should lower vacuum_freeze_min_age to something sane, like 100.

Currently, the defaults are 50m for vacuum_freeze_min_age, 150m for
vacuum_freeze_table_age, and 200m for autovacuum_freeze_max_age, which
doesn't seem like it matches what you're saying.
vacuum_freeze_min_age doesn't force anti-wraparound vacuuming anyway;
it just controls the tuple age at which opportunistic freezing starts.
 There's possibly some advantage to lowering it anyway, in the hopes
of leaving less work for the eventual full-table scan to do, but
dropping it to as low as 1m seems like it might be too much of a good
thing - we might end up freezing a lot of tuples that would have
gotten removed before they hit the 50m freeze threshold we have today.

While we're talking about prioritizing vacuum, though, it would be
nice to have a high-priority anti-wraparound vacuum and a low-priority
anti-wraparound vacuum.  When a table crosses the "low priority"
threshold, we work on freezing it *if there is no other vacuuming that
needs doing* - i.e. we're the only autovacuum process running, and no
other table needs autovacuuming either for dead tuples or for
wraparound.  When it crosses the "high priority" threshold, then we
behave as now, or perhaps even more aggressively (move to front of
queue, raise or cost delay or ignore it outright, etc.).  The "low
priority" anti-wraparound vacuum would abort if some other vacuuming
came to be required, if a lock conflict occurred, etc., and might also
run with a higher cost delay.  I believe this would tend to spread the
anti-wraparound work out over a longer period of time, instead of
clumping it all together as often happens today, and reduce the effect
it has on foreground activities.  It might not be going far enough in
that direction but at least it would be a start.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] lazy_vacuum_heap()'s removal of HEAPTUPLE_DEAD tuples

[Noah Misch]

On Mon, Jan 28, 2013 at 02:12:33PM +, Simon Riggs wrote:
> On 23 January 2013 04:35, Noah Misch  wrote:
> >> Also, perhaps we should
> >> consider Simon's one-liner fix for backpatching this instead of the
> >> original patch you posted?
> >
> > I have no nontrivial preference between the two approaches.
> 
> Sorry, I didn't see this. I guess you saw I applied my one liner and
> backpatched it.

Yes; thanks.

> I'm expecting to apply Noah's larger patch to HEAD with the suggested
> edits. I'll do that last thing in the CF.

What "suggested edits" do you have in mind?


-- 
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] Hm, table constraints aren't so unique as all that

[Tom Lane]

Peter Geoghegan  writes:
> On 29 January 2013 00:25, Tom Lane  wrote:
>> Of course this wouldn't be material for back-patching, but it seems to
>> me there's still time to fix this for 9.3, and we should do so if we
>> want to claim that the enhanced-errors patch uniquely identifies
>> constraints.

> I can see the case for fixing this, but I don't feel that it's
> particularly important that constraints be uniquely identifiable from
> the proposed new errdata fields.

I think that we'll soon be buried in gripes if they're not.  Pretty much
the whole point of this patch is to allow applications to get rid of
ad-hoc, it-usually-works coding techniques.  I'd argue that not checking
the entire constraint identity is about as fragile as trying to "sed"
the constraint name out of a potentially-localized error message.
In both cases, it often works fine, until the application's context
changes.

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] autovacuum not prioritising for-wraparound tables

[Robert Haas]

On Sun, Jan 27, 2013 at 2:17 PM, Jeff Janes  wrote:
> On Fri, Jan 25, 2013 at 9:19 AM, Robert Haas  wrote:
>> I think that to do this right, we need to consider not only the status
>> quo but the trajectory.  For example, suppose we have two tables to
>> process, one of which needs a wraparound vacuum and the other one of
>> which needs dead tuples removed.  If the table needing the wraparound
>> vacuum is small and just barely over the threshold, it isn't urgent;
>
> But it being small, it also won't take long to vacuum.  Why not just do it?

Because "big" and "small" are relative terms.

>> but if it's large and way over the threshold, it's quite urgent.
>> Similarly, if the table which needs dead tuples removed is rarely
>> updated, postponing vacuum is not a big deal, but if it's being
>> updated like crazy, postponing vacuum is a big problem.
>
> I don't see this as being the case.  If it is being updated like
> crazy, it doesn't matter whether it meets the threshold to have tuples
> removed *right at the moment* or not.  It will meet that threshold
> soon.  If you can't keep up with that need with your current settings,
> you have a steady-state problem.  Changing the order, or not changing
> the order, isn't going to make a whole lot of difference, you need to
> overcome the steady-state problem.

Sure.  There are many people for which vacuum has no trouble at all
keeping up, and others for whom it isn't even close to keeping up.
People in the first category aren't likely to be damaged by the
proposed change and people in the second category aren't likely to be
helped.  The issue is around what happens for people who are close to
the edge.  Will things get better or worse?  Alvaro (and Simon)
content that there will be cases where full-cluster shutdowns that
happen under today's algorithm would be avoided if we prioritize
anti-wraparound vacuums over dead-tuple-cleanup vacuums.  I believe
that.  I also believe that there will be cases where it goes the other
way - where a bloat situation that remains under control with today's
algorithm gets just perturbed just enough by this change to cause
runaway table bloat.  Or at least, I contend that we don't have nearly
enough evidence that that *won't* happen to risk back-patching a
change of this type.

In my experience, full-cluster shutdowns caused by autovacuum failing
to advance datfrozenxid are extremely rare - and if they do happen,
it's usually because the vacuum cost delay is set too high, or the
cost limit too low.  If we want to attack the problem of making sure
such shutdowns don't happen, I'd argue that the most promising way to
attack that problem is to progressively ratchet the delay down and the
cost limit up as age(relfrozenxid) gets larger.  On the other hand,
problems with runaway table bloat are relatively common.  Heikki's
8.4-era changes have of course helped quite a bit, but the problem is
still very, very common.  All you need is a series of "long"-running
transactions (like a couple of *minutes* on a busy system), or a
vacuum cost delay that is just ever-so-slightly too high, and you're
completely hosed.  I agree with you that if you've got a database
that's well-tuned, so that you aren't skating on the ragged edge of
disaster, this change probably won't break anything.  But I am willing
to bet that there are people out there who are, completely
unknowingly, skating on that ragged edge.  It is not as if we provide
an easy way to know whether you've got the cost delay set optimally.

>> Categorically
>> putting autovacuum wraparound tables ahead of everything else seems
>> simplistic, and thinking that more dead tuples is more urgent than
>> fewer dead tuples seems *extremely* simplistic.
>>
>> I ran across a real-world case where a user had a small table that had
>> to be vacuumed every 15 seconds to prevent bloat.  If we change the
>> algorithm in a way that gives other things priority over that table,
>
> Eventually an anti-wrap around is going to be done, and once it starts
> it does have priority, because things already underway don't get
> preempted.  Have they ever reached that point?  Did it cause problems?

In that specific case, I don't know.

>> then that user could easily get hosed when they install a maintenance
>> release containing this change.
>
> Yeah, I don't know that back-patching is a good idea, or at least not soon.

That's all I'm arguing.  I think it would be nice to do something for
9.3, preferably a little more sophisticated than just "put all
anti-wraparound vacuums first".

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_ctl idempotent option

[Bruce Momjian]

On Mon, Jan 28, 2013 at 09:29:35PM -0500, Bruce Momjian wrote:
> On Mon, Jan 28, 2013 at 03:40:08PM +, Simon Riggs wrote:
> > On 14 January 2013 15:29, Alvaro Herrera  wrote:
> > > Tom Lane wrote:
> > >> Peter Eisentraut  writes:
> > >> > Here is a patch to add an option -I/--idempotent to pg_ctl, the result
> > >> > of which is that pg_ctl doesn't error on start or stop if the server is
> > >> > already running or already stopped.
> > >>
> > >> Idempotent is a ten-dollar word.  Can we find something that average
> > >> people wouldn't need to consult a dictionary to understand?
> > >
> > > --no-error perhaps?
> > 
> > 
> > I think --force  would be the accepted way to ensure something happens
> > as specified
> > 
> > 
> > Mind you, I'm not sure I see the value in throwing an error if the
> > server is in the desired state already. Who actually wants that
> > behaviour? Can't we just change the behaviour? Existing scripts would
> > still work, since we are simply skipping an error.
> 
> pg_upgrade uses that to find out of the server was already running or if
> we started it.  This is to start the server to remove the
> postmaster.pid file.  Also, no one has explained how not knowing if -o
> options were used was a safe.

OK, I had some time to think about this.  Basically, we have three
outcomes for pg_ctl start:

server not running and pg_ctl start success
server start failed
server already running

Can't we just assign different return values to these cases, e.g. 0, 1,
2?  We already print output telling the user what happened.

I don't think I like --force because it isn't clear if we are forcing
the start to have done something, or forcing the server to be running.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patches for TODO item: Avoid truncating empty OCDR temp tables on COMMIT

[Robert Haas]

On Mon, Jan 28, 2013 at 8:39 AM, Heikki Linnakangas
 wrote:
> On 15.01.2013 21:03, Tom Lane wrote:
>> Robert Haas  writes:
>>>
>>> On the third hand, the fact that a table is OCDR is recorded in
>>> backend-local storage somewhere, and that storage (unlike the
>>> relcache) had better be reliable.  So maybe there's some way to
>>> finesse it that way.
>>
>> Hm, keep a flag in that storage you mean?  Yeah, that could possibly
>> work.
>
> Sounds reasonable.
>
> Until someone gets around to write a patch along those lines, I'm inclined
> to apply this one liner:
>
> *** a/src/backend/commands/tablecmds.c
> --- b/src/backend/commands/tablecmds.c
> ***
> *** 10124,10130  PreCommit_on_commit_actions(void)
> /* Do nothing (there shouldn't be such
> entries, actually) */
> break;
> case ONCOMMIT_DELETE_ROWS:
> !   oids_to_truncate =
> lappend_oid(oids_to_truncate, oc->relid);
> break;
> case ONCOMMIT_DROP:
> {
> --- 10124,10136 
> /* Do nothing (there shouldn't be such
> entries, actually) */
> break;
> case ONCOMMIT_DELETE_ROWS:
> !   /*
> !* If this transaction hasn't accessed any
> temporary relations,
> !* we can skip truncating ON COMMIT DELETE
> ROWS tables, as
> !* they must still be empty.
> !*/
> !   if (MyXactAccessedTempRel)
> !   oids_to_truncate =
> lappend_oid(oids_to_truncate, oc->relid);
> break;
> case ONCOMMIT_DROP:
> {
>
> We already have that MyXactAccessedTempRel global flag. Just checking that
> should cover many common cases.

+1 for that.  I'm actually unconvinced that we need to do any better
than that in general.  But certainly that seems like a good first
step.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] unlogged tables vs. GIST

[Robert Haas]

On Mon, Jan 28, 2013 at 4:04 AM, Heikki Linnakangas
 wrote:
> Do we need to do anything to unloggedLSN in pg_resetxlog?

Does the server go into recovery after pg_resetxlog?  If so, no.  If
not, probably, but I have no idea what.  There's no "safe" value in
that case; what we ought to do is force a reset of all unlogged
relations, or just punt and hope nothing bad happens (which after all
is what pg_resetxlog is mostly about anyway).

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] allowing privileges on untrusted languages

[Robert Haas]

On Sun, Jan 27, 2013 at 11:15 PM, Craig Ringer  wrote:
> On 01/28/2013 02:15 AM, Robert Haas wrote:
>
> I am not sure whether it's really true that a capability mechanism
> could "never really satisfy" anyone.  It worked for Linux.
>
> I have no concern about using a capabilities approach for this, but I don't
> think Linux is a great example here. Linux's capabilities have been defined
> in a somewhat ad-hoc fashion and a huge amount of stuff is bundled into
> CAP_SYS_ADMIN. Several capabilities provide escalation routes to root /
> CAP_SYS_ADMIN. See:
>
> https://lwn.net/Articles/486306/
> http://dl.packetstormsecurity.net/papers/attack/exploiting_capabilities_the_dark_side.pdf
>
> There's nothing wrong with capability systems, it's just clear that they
> need to be designed, documented and maintained carefully. Adding ad-hoc
> capbilities is exactly the wrong route to take, and will lead us into the
> same mess Linux is in now.
>
> But, I think event triggers are a credible answer, too, and they
> certainly are more flexible.
>
> Yes,  but with the caveat that leaving security design to user triggers will
> provide users with more opportunities for error - failure to think about
> schemas and search_path, testing role membership via some hacked-together
> queries instead of the built-in system information functions, failure to
> consider SECURITY DEFINER and the effect of session_user vs current_user,
> etc. Some docs on writing security triggers and some standard triggers in an
> extension module would go a long way to mitigating that, though. The appeal
> of the trigger based approach is that it means core doesn't land up needing
> CAP_CAN_EXECUTE_PLPERLU_ON_TUESDAYS_AFTER_MIDDAY_ON_A_FULL_MOON_IN_A_LEAPYEAR.

+1 to the entire email, and well said.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Enabling Checksums

[Robert Haas]

On Sun, Jan 27, 2013 at 5:28 PM, Jeff Davis  wrote:
> There's a maximum of one FPI per page per cycle, and we need the FPI for
> any modified page in this design regardless.
>
> So, deferring the XLOG_HINT WAL record doesn't change the total number
> of FPIs emitted. The only savings would be on the trivial XLOG_HINT wal
> record itself, because we might notice that it's not necessary in the
> case where some other WAL action happened to the page.

OK, I see.  So the case where this really hurts is where a page is
updated for hint bits only and then not touched again for the
remainder of the checkpoint cycle.

>> > At first glance, it seems sound as long as the WAL FPI makes it to disk
>> > before the data. But to meet that requirement, it seems like we'd need
>> > to write an FPI and then immediately flush WAL before cleaning a page,
>> > and that doesn't seem like a win. Do you (or Simon) see an opportunity
>> > here that I'm missing?
>>
>> I am not sure that isn't a win.  After all, we can need to flush WAL
>> before flushing a buffer anyway, so this is just adding another case -
>
> Right, but if we get the WAL record in earlier, there is a greater
> chance that it goes out with some unrelated WAL flush, and we don't need
> to flush the WAL to clean the buffer at all. Separating WAL insertions
> from WAL flushes seems like a fairly important goal, so I'm a little
> skeptical of a proposal to narrow that gap so drastically.
>
> It's hard to analyze without a specific proposal on the table. But if
> cleaning pages requires a WAL record followed immediately by a flush, it
> seems like that would increase the number of actual WAL flushes we need
> to do by a lot.

Yeah, maybe.  I think Simon had a good argument for not pursuing this
route, anyway.

>> and the payoff is that the initial access to a page, setting hint
>> bits, is quickly followed by a write operation, we avoid the need for
>> any extra WAL to cover the hint bit change.  I bet that's common,
>> because if updating you'll usually need to look at the tuples on the
>> page and decide whether they are visible to your scan before, say,
>> updating one of them
>
> That's a good point, I'm just not sure how avoid that problem without a
> lot of complexity or a big cost. It seems like we want to defer the
> XLOG_HINT WAL record for a short time; but not wait so long that we need
> to clean the buffer or miss a chance to piggyback on another WAL flush.
>
>> > By the way, the approach I took was to add the heap buffer to the WAL
>> > chain of the XLOG_HEAP2_VISIBLE wal record when doing log_heap_visible.
>> > It seemed simpler to understand than trying to add a bunch of options to
>> > MarkBufferDirty.
>>
>> Unless I am mistaken, that's going to heavy penalize the case where
>> the user vacuums an insert-only table.  It will emit much more WAL
>> than currently.
>
> Yes, that's true, but I think that's pretty fundamental to this
> checksums design (and of course it only applies if checksums are
> enabled). We need to make sure an FPI is written and the LSN bumped
> before we write a page.
>
> That's why I was pushing a little on various proposals to either remove
> or mitigate the impact of hint bits (load path, remove PD_ALL_VISIBLE,
> cut down on the less-important hint bits, etc.). Maybe those aren't
> viable, but that's why I spent time on them.
>
> There are some other options, but I cringe a little bit thinking about
> them. One is to simply exclude the PD_ALL_VISIBLE bit from the checksum
> calculation, so that a torn page doesn't cause a problem (though
> obviously that one bit would be vulnerable to corruption). Another is to
> use a double-write buffer, but that didn't seem to go very far. Or, we
> could abandon the whole thing and tell people to use ZFS/btrfs/NAS/SAN.

I am inclined to think that we shouldn't do any of this stuff for now.
 I think it's OK if the first version of checksums is
not-that-flexible and/or not-that-performant.  We can optimize for
those things later.  Trying to monkey with this at the same time we're
trying to get checksums in risks introducing new diverting focus from
getting checksums done at all, and risks also introducing new data
corruption bugs.  We have a reputation of long standing for getting it
right first and then getting it to perform well later, so it shouldn't
a be a total shock if we take that approach here, too.  I see no
reason to think that the performance problems must be solved up front
or not at all.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_ctl idempotent option

[Bruce Momjian]

On Mon, Jan 28, 2013 at 03:40:08PM +, Simon Riggs wrote:
> On 14 January 2013 15:29, Alvaro Herrera  wrote:
> > Tom Lane wrote:
> >> Peter Eisentraut  writes:
> >> > Here is a patch to add an option -I/--idempotent to pg_ctl, the result
> >> > of which is that pg_ctl doesn't error on start or stop if the server is
> >> > already running or already stopped.
> >>
> >> Idempotent is a ten-dollar word.  Can we find something that average
> >> people wouldn't need to consult a dictionary to understand?
> >
> > --no-error perhaps?
> 
> 
> I think --force  would be the accepted way to ensure something happens
> as specified
> 
> 
> Mind you, I'm not sure I see the value in throwing an error if the
> server is in the desired state already. Who actually wants that
> behaviour? Can't we just change the behaviour? Existing scripts would
> still work, since we are simply skipping an error.

pg_upgrade uses that to find out of the server was already running or if
we started it.  This is to start the server to remove the
postmaster.pid file.  Also, no one has explained how not knowing if -o
options were used was a safe.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical changeset generation v4

[Andres Freund]

On 2013-01-28 12:23:02 +0100, Andres Freund wrote:
> On 2013-01-27 12:28:21 -0500, Steve Singer wrote:
> > On 13-01-22 11:30 AM, Andres Freund wrote:
> > >Hi,
> > >
> > >I pushed a new rebased version (the xlogreader commit made it annoying
> > >to merge).
> > >
> > >The main improvements are
> > >* way much coherent code internally for intializing logical rep
> > >* explicit control over slots
> > >* options for logical replication
> > 
> > 
> > Exactly what is the syntax for using that.  My reading your changes to
> > repl_gram.y make me think that any of the following should work (but they
> > don't).
> > 
> > START_LOGICAL_REPLICATION 'slon1' 0/0 ('opt1')
> >  ERROR:  syntax error: unexpected character "("
> > 
> > "START_LOGICAL_REPLICATION 'slon1' 0/0 ('opt1' 'val1')
> >  ERROR:  syntax error: unexpected character "("
> > 
> > START_LOGICAL_REPLICATION 'slon1' 0/0 ('opt1','opt2')
> > ERROR:  syntax error: unexpected character "("
> 
> The syntax is right, the grammar (or rather scanner) support is a bit
> botched, will push a new version soon.

Pushed and rebased some minutes ago. I changed the syntax so that slot
names, plugins, and option names are identifiers and behave just as in
normal sql identifier. That means ' need to be changed to ".

The new version is rebased ontop of fklocks, walsender et al, which was
a bit of work but actually makes more comprehensive logging in
heap_update easier. That will come tomorrow.

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] Hm, table constraints aren't so unique as all that

[Peter Geoghegan]

On 29 January 2013 00:25, Tom Lane  wrote:
> Of course this wouldn't be material for back-patching, but it seems to
> me there's still time to fix this for 9.3, and we should do so if we
> want to claim that the enhanced-errors patch uniquely identifies
> constraints.

I can see the case for fixing this, but I don't feel that it's
particularly important that constraints be uniquely identifiable from
the proposed new errdata fields.

-- 
Regards,
Peter Geoghegan


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Hm, table constraints aren't so unique as all that

[Tom Lane]

Over in the thread about enhanced error fields, I claimed that
"constraints are uniquely named among those associated with a table,
or with a domain".  But it turns out that that ain't necessarily so,
because the code path for index constraints doesn't pay any attention
to pre-existing check constraints:

d1=# create table t1 (f1 int);
CREATE TABLE
d1=# alter table t1 add constraint c1 check (f1 > 0);
ALTER TABLE
d1=# alter table t1 add constraint c1 unique (f1);
ALTER TABLE
d1=# \d t1
  Table "public.t1"
 Column |  Type   | Modifiers 
+-+---
 f1 | integer | 
Indexes:
"c1" UNIQUE CONSTRAINT, btree (f1)
Check constraints:
"c1" CHECK (f1 > 0)

If you do this in the other order it does get rejected:

d1=# create table t2 (f1 int);
CREATE TABLE
d1=# alter table t2 add constraint c2 unique (f1);
ALTER TABLE
d1=# alter table t2 add constraint c2 check (f1 > 0);
ERROR:  constraint "c2" for relation "t2" already exists

Aside from being plain inconsistent, this seems to me to create a
dump/reload hazard: pg_dump has no idea that it would have to dump
these two constraints in a particular order to make them reloadable.

In practice there's not such a big risk because pg_dump prefers to stick
CHECK constraints directly into the CREATE TABLE rather than add them
after-the-fact.  But if it had to split off the CHECK constraint to
avoid a circularity problem, I don't believe there's anything preventing
a reload failure.

I think we need to tighten this down by having index-constraint creation
check for conflicts with other constraint types.  It also seems like it
might be a good idea to put in a unique index to enforce the intended
lack of conflicts --- note that the existing index on (conname,
connamespace) isn't unique.  It's a bit problematic that pg_constraint
contains both table-related constraints and domain-related constraints,
but it strikes me that we could get close enough by changing
pg_constraint_conname_nsp_index to be a unique index on
(conname, connamespace, conrelid, contypid).  That would fix the problem
as long as no pg_constraint entry ever has both conrelid and contypid
nonzero; the unique index couldn't catch such an error.  But it doesn't
seem to me that such a coding error would escape detection anyway.

Of course this wouldn't be material for back-patching, but it seems to
me there's still time to fix this for 9.3, and we should do so if we
want to claim that the enhanced-errors patch uniquely identifies
constraints.

Thoughts?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] error C2011 in Visual Studio 2012

[Craig Ringer]

On 01/27/2013 01:14 AM, Anderson C. Carniel wrote:
> Hi,
>
> I'm trying to build a. dll file to extend the postgres server with C
> functions. I'm using visual studio 2012 to build the dll, and
> PostgreSQL 9.2. I imported all directories postgres "\include\server*"
> But I'm having the errors:
>
> error C2011: 'timezone': 'struct' type redefinition
>
> error C2011: 'itimerval': 'struct' type redefinition
>
> In the file *pg_confi_os.h* at line 205 and 214
>

First, you'll want to build with Visual Studio 2010 or older. The
Express edition works fine.

Second, it's very unlikely that you can just compile the extension .c
file stand-alone. You'll need to set a bunch of preprocesor definitions,
some of which are somewhat configuration/environment specific.

The best way to compile extensions at the moment may be to get the
PostgreSQL source tree, add your extension as a folder inside the
contrib/ directory with a Makefile, then use the tooling in
src/tools/msvc to compile the source tree including the extension.

I would really like to see this improve, with a working PGXS alternative
for Windows MSVC builds. I'm not presently aware of anything, but I
haven't investigated building extensions out of tree under Windows/MSVC
in detail yet.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] Event Triggers: adding information

[Dimitri Fontaine]

Christopher Browne  writes:
> I'm poking at event triggers a bit; would like to set up some examples
> (and see if they
> work, or break down badly; both are interesting results) to do some
> validation of schema
> for Slony.

Cool, thanks!

> What I'm basically thinking about is to set up some event triggers that run on
> DROP TABLE / DROP SEQUENCE, and see about cleaning up the replication
> side of things (e.g. - inject a request to drop the table/sequence
> from replication).

Sure. In what got commited from the current patch series, you will only
know that a DROP TABLE (or DROP SEQUENCE) occured, and we're trying to
get to an agreement with Robert if we should prefer to add visibility to
such events that occurs in a CASCADE statement or rather add the OID
(and maybe the name) of the Object that's going to be dropped.

Your opinion is worth a lot on that matter, if you have one to share! :)

> I have a bit of a complaint as to what documentation is included; I don't see
> any references in the documentation to ddl_command_start / ddl_command_end,
> which seem to be necessary values for event triggers.

What we have now here:

  http://www.postgresql.org/docs/devel/static/event-trigger-matrix.html
  http://www.postgresql.org/docs/devel/static/sql-createeventtrigger.html
  
http://www.postgresql.org/docs/devel/static/plpgsql-trigger.html#PLPGSQL-EVENT-TRIGGER

Is it not visible enough, or really missing the point?

> I'd tend to think that there should be a new subsection in the "man page" for
> CREATE TRIGGER that includes at least two fully formed examples of event
> triggers, involving the two events in question.  Is change of that
> sort in progress?

The event triggers are addressed in a whole new chapter of the docs,
maybe that's why you didn't find the docs?

-- 
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

Re: [HACKERS] Event Triggers: adding information

[Christopher Browne]

I'm poking at event triggers a bit; would like to set up some examples
(and see if they
work, or break down badly; both are interesting results) to do some
validation of schema
for Slony.

What I'm basically thinking about is to set up some event triggers that run on
DROP TABLE / DROP SEQUENCE, and see about cleaning up the replication
side of things (e.g. - inject a request to drop the table/sequence
from replication).

I have a bit of a complaint as to what documentation is included; I don't see
any references in the documentation to ddl_command_start / ddl_command_end,
which seem to be necessary values for event triggers.

I'd tend to think that there should be a new subsection in the "man page" for
CREATE TRIGGER that includes at least two fully formed examples of event
triggers, involving the two events in question.  Is change of that
sort in progress?
-- 
When confronted by a difficult problem, solve it by reducing it to the
question, "How would the Lone Ranger handle this?"


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] error C2011 in Visual Studio 2012

[Andrew Dunstan]

On 01/26/2013 12:14 PM, Anderson C. Carniel wrote:

Hi,

I'm trying to build a. dll file to extend the postgres server with C 
functions. I'm using visual studio 2012 to build the dll, and 
PostgreSQL 9.2. I imported all directories postgres "\include\server*" 
But I'm having the errors:




In the first place, Note that VS2012 is not (yet) a supported build 
environment for Postgres. See 



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] logical changeset generation v4

[Steve Singer]

On 13-01-28 06:23 AM, Andres Freund wrote:
The CF is also there to find UI warts and such, so something like this 
seems perfectly fine. Even moreso as it doesn't look this will get 
into 9.3 anyway. I wanted to add such an option, but I was too 
lazy^Wbusy to think about the sematics. Your current syntax doesn't 
really allow arguments to be specified in a nice way. I was thinking 
of -o name=value and allowing multiple specifications of -o to build 
the option string. Any arguments against that?


Multiple -o options sound fine to me.



/* Initiate the replication stream at specified location */
-   snprintf(query, sizeof(query), "START_LOGICAL_REPLICATION '%s' %X/%X",
-slot, (uint32) (startpos >> 32), (uint32) startpos);
+   snprintf(query, sizeof(query), "START_LOGICAL_REPLICATION '%s' %X/%X 
(%s)",
+slot, (uint32) (startpos >> 32), (uint32) 
startpos,plugin_opts);

ISTM that (%s) shouldn't be specified when there are no options, but as
the options need to be pre-escaped anyway, that looks like a non-problem
in a bit more complete implementation.

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] enhanced error fields

[Peter Geoghegan]

On 28 January 2013 21:33, Peter Eisentraut  wrote:
> Another point, in case someone wants to revisit this in the future, is
> that these fields were applied in a way that is contrary to the SQL
> standard, I think.
>
> The presented patch interpreted ROUTINE_NAME as: the error happened
> while executing this function.  But according to the standard, the field
> is only set when the error was directly related to the function itself,
> for example when calling an INSERT statement in a non-volatile function.

Right. It seems to me that ROUTINE_NAME is vastly less compelling than
the fields that are likely to be present in the committed patch. GET
DIAGNOSTICS, as implemented by DB2, allows clients /to poll/ for a
large number of fields. I'm not really interested in that myelf, but
if we were to add something in the same spirit, I think that extending
errdata to support this would not be a sensible approach.

Perhaps I'm mistaken, but I can't imagine that it would be terribly
useful to anyone (including Pavel) to have a GET DIAGNOSTICS style
ROUTINE_NAME.

-- 
Regards,
Peter Geoghegan


-- 
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] logical changeset generation v4 - Heikki's thoughts about the patch state

[Steve Singer]

On 13-01-28 06:17 AM, Andres Freund wrote:

Hi,

3.  Pass the delete (with no key values) onto the replication client and let
it deal with it (see 1 and 2)
Hm.


While I agree that nicer behaviour would be good I think the real
enforcement should happen on a higher level, e.g. with event triggers or
somesuch. It seems way too late to do anything about it when we're
already decoding. The transaction will already have committed...


Ideally the first line of enforcement would be with event triggers. The 
thing with user-level mechanisms for enforcing things is that they 
sometimes can be disabled or by-passed.  I don't have a lot of sympathy 
for people who do this but I like the idea of at least having the option 
coding defensively to detect the situation and whine to the user.



How do you plan on dealing with sequences?
I don't see my plugin being called on sequence changes and I don't see
XLOG_SEQ_LOG listed in DecodeRecordIntoReorderBuffer.  Is there a reason why
this can't be easily added?

I basically was hoping for Simon's sequence-am to get in before doing
anything real here. That didn't really happen yet.
I am not sure whether there's a real usecase in decoding normal
XLOG_SEQ_LOG records, their content isn't all that easy to interpet
unless youre rather familiar with pg's innards.

So, adding support wouldn't hard from a technical pov but it seems the
semantics are a bit hard to nail down.


Also what do we want to do about TRUNCATE support.  I could always leave a
TRUNCATE trigger in place that logged the truncate to a sl_truncates and
have my replication daemon respond to the insert on a   sl_truncates table
by actually truncating the data on the replica.

I have planned to add some generic "table_rewrite" handling, but I have
to admit I haven't thought too much about it yet. Currently if somebody
rewrites a table, e.g. with an ALTER ... ADD COLUMN .. DEFAULT .. or
ALTER COLUMN ... USING ..., you will see INSERTs into a temporary
table. That basically seems to be a good thing, but the user needs to be
told about that ;)


I've spent some time this weekend updating my prototype plugin that
generates slony 2.2 style COPY output.  I have attached my progress here
(also https://github.com/ssinger/slony1-engine/tree/logical_repl).  I have
not gotten as far as modifying slon to act as a logical log receiver, or
made a version of the slony apply trigger that would process these
changes.

I only gave it a quick look and have a couple of questions and
remarks. The way you used the options it looks like youre thinking of
specifying all the tables as options? I would have thought those would
get stored & queried locally and only something like the 'replication
set' name or such would be set as an option.


The way slony works today is that the list of tables to pull for a SYNC 
comes from the subscriber because the subscriber might be behind the 
provider, where a table has been removed from the set in the meantime.  
The subscriber still needs to receive data from that table until it is 
caught up to the point where that removal happens.


Having a time-travelled version of a user table (sl_table) might fix 
that problem but I haven't yet figured out how that needs to work with 
cascading (since that is a feature of slony today I can't ignore the 
problem). I'm also not sure how that will work with table renames.  
Today if the user renames a table inside of an EXECUTE SCRIPT slony will 
update the name of the table in sl_table.   This type of change wouldn't 
be visible (yet) in the time-travelled catalog.  There might be a 
solution to this yet but I haven't figured out it.  Sticking with what 
slony does today seemed easier as a first step.



Iterating over a list with
for(i = 0; i < options->length; i= i + 2 )
{
DefElem * def_schema = (DefElem*) list_nth(options,i);
is not a good idea btw, thats quadratic in complexity ;)


Thanks I'll rewrite this to walk a list of  ListCell objects with next.



In the REORDER_BUFFER_CHANGE_UPDATE I suggest using
relation->rd_primary, just as in the DELETE case, that should always
give you a consistent candidate key in an efficient manner.


I haven't looked into the details of what is involved in setting up a
subscription with the snapshot exporting.

That hopefully shouldn't be too hard... At least thats the idea :P


I couldn't get the options on the START REPLICATION command to parse so I
just hard coded some list building code in the init method. I do plan on
pasing the list of tables to replicate from the replica to the plugin
(because this list comes from the replica).   Passing what could be a few
thousand table names as a list of arguments is a bit ugly and I admit my
list processing code is rough.  Does this make us want to reconsider the
format of the option_list ?

Yea, something's screwed up there, sorry. Will push a fix later today.


I guess should provide an opinion on if I think that the patch in this CF,
if commi

Re: [HACKERS] enhanced error fields

[Peter Eisentraut]

On 1/5/13 12:48 PM, Peter Geoghegan wrote:
>> is there agreement of routine_name and trigger_name fields?
> Well, Tom and I are both opposed to including those fields. Peter E
> seemed to support it in some way, but didn't respond to Tom's
> criticisms (which were just a restatement of my own). So, it seems to
> me that we're not going to do that, assuming nothing changes.

Another point, in case someone wants to revisit this in the future, is
that these fields were applied in a way that is contrary to the SQL
standard, I think.

The presented patch interpreted ROUTINE_NAME as: the error happened
while executing this function.  But according to the standard, the field
is only set when the error was directly related to the function itself,
for example when calling an INSERT statement in a non-volatile function.
 This is consistent with how, for example, TABLE_NAME is set when the
error is about the table, not just happened while reading the table.



-- 
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] [PERFORM] pgbench to the MAXINT

[Gurjeet Singh]

On Sat, Jan 26, 2013 at 11:24 PM, Satoshi Nagayasu  wrote:

> Hi,
>
> I have reviewed this patch.
>
> https://commitfest.postgresql.org/action/patch_view?id=1068
>
> 2012/12/21 Gurjeet Singh :
> > The patch is very much what you had posted, except for a couple of
> > differences due to bit-rot. (i) I didn't have to #define
> MAX_RANDOM_VALUE64
> > since its cousin MAX_RANDOM_VALUE is not used by code anymore, and (ii) I
> > used ternary operator in DDLs[] array to decide when to use bigint vs int
> > columns.
> >
> > Please review.
> >
> > As for tests, I am currently running 'pgbench -i -s 21474' using
> > unpatched pgbench, and am recording the time taken;Scale factor 21475 had
> > actually failed to do anything meaningful using unpatched pgbench. Next
> I'll
> > run with '-s 21475' on patched version to see if it does the right thing,
> > and in acceptable time compared to '-s 21474'.
> >
> > What tests would you and others like to see, to get some confidence
> in
> > the patch? The machine that I have access to has 62 GB RAM, 16-core
> > 64-hw-threads, and about 900 GB of disk space.
>
> I have tested this patch, and hvae confirmed that the columns
> for aid would be switched to using bigint, instead of int,
> when the scalefactor >= 20,000.
> (aid columns would exeed the upper bound of int when sf>21474.)
>
> Also, I added a few fixes on it.
>
> - Fixed to apply for the current git master.
> - Fixed to surpress few more warnings about INT64_FORMAT.
> - Minor improvement in the docs. (just my suggestion)
>
> I attached the revised one.
>

Looks good to me. Thanks!

-- 
Gurjeet Singh

http://gurjeet.singh.im/

Re: [HACKERS] Performance Improvement by reducing WAL for Update Operation

[Heikki Linnakangas]

On 28.01.2013 15:39, Amit Kapila wrote:

Rebased the patch as per HEAD.


I don't like the way heap_delta_encode has intimate knowledge of how the 
lz compression works. It feels like a violent punch through the 
abstraction layers.


Ideally, you would just pass the old and new tuple to pglz as char *, 
and pglz code would find the common parts. But I guess that's too slow, 
as that's what I originally suggested and you rejected that approach. 
But even if that's not possible on performance grounds, we don't need to 
completely blow up the abstraction. pglz can still do the encoding - the 
caller just needs to pass it the attribute boundaries to consider for 
matches, so that it doesn't need to scan them byte by byte.


I came up with the attached patch. I wrote it to demonstrate the API, 
I'm not 100% sure the result after decoding is correct.


- Heikki
diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
index e39b977..bbdee4f 100644
--- a/src/backend/access/common/heaptuple.c
+++ b/src/backend/access/common/heaptuple.c
@@ -60,7 +60,11 @@
 #include "access/sysattr.h"
 #include "access/tuptoaster.h"
 #include "executor/tuptable.h"
+#include "utils/datum.h"
+#include "utils/pg_lzcompress.h"
 
+/* guc variable for EWT compression ratio*/
+int			wal_update_compression_ratio = 25;
 
 /* Does att's datatype allow packing into the 1-byte-header varlena format? */
 #define ATT_IS_PACKABLE(att) \
@@ -617,6 +621,119 @@ heap_copytuple_with_tuple(HeapTuple src, HeapTuple dest)
 	memcpy((char *) dest->t_data, (char *) src->t_data, src->t_len);
 }
 
+
+/* 
+ * heap_delta_encode
+ *
+ *		Calculate the delta between two tuples, using pglz. The result is
+ * stored in *encdata. *encdata must point to a PGLZ_header buffer, with at
+ * least PGLZ_MAX_OUTPUT(newtup->t_len) bytes.
+ * 
+ */
+bool
+heap_delta_encode(TupleDesc tupleDesc, HeapTuple oldtup, HeapTuple newtup,
+  char *encdata)
+{
+	HeapTupleHeader tup = oldtup->t_data;
+	Form_pg_attribute *att = tupleDesc->attrs;
+	bool		hasnulls = HeapTupleHasNulls(oldtup);
+	bits8	   *bp = oldtup->t_data->t_bits;		/* ptr to null bitmap in tuple */
+	bool		slow = false;	/* can we use/set attcacheoff? */
+	char	   *tp;/* ptr to tuple data */
+	long		off;			/* offset in tuple data */
+	int			natts;
+	int32	   *offsets;
+	int			noffsets;
+	int			attnum;
+	PGLZ_Strategy strategy;
+
+	/*
+	 * Loop through all attributes, if the attribute is modified by the update
+	 * operation, store the [Offset,Length] reffering old tuple version till
+	 * the last unchanged column in the EWT as History Reference, else store
+	 * the [Length,Data] from new tuple version as New Data.
+	 */
+	natts = HeapTupleHeaderGetNatts(oldtup->t_data);
+
+	offsets = palloc(natts * sizeof(int32));
+
+	noffsets = 0;
+
+	/* copied from heap_deform_tuple */
+	tp = (char *) tup + tup->t_hoff;
+	off = 0;
+	for (attnum = 0; attnum < natts; attnum++)
+	{
+		Form_pg_attribute thisatt = att[attnum];
+
+		if (hasnulls && att_isnull(attnum, bp))
+		{
+			slow = true;		/* can't use attcacheoff anymore */
+			continue;
+		}
+
+		if (!slow && thisatt->attcacheoff >= 0)
+			off = thisatt->attcacheoff;
+		else if (thisatt->attlen == -1)
+		{
+			/*
+			 * We can only cache the offset for a varlena attribute if the
+			 * offset is already suitably aligned, so that there would be no
+			 * pad bytes in any case: then the offset will be valid for either
+			 * an aligned or unaligned value.
+			 */
+			if (!slow &&
+off == att_align_nominal(off, thisatt->attalign))
+thisatt->attcacheoff = off;
+			else
+			{
+off = att_align_pointer(off, thisatt->attalign, -1,
+		tp + off);
+slow = true;
+			}
+		}
+		else
+		{
+			/* not varlena, so safe to use att_align_nominal */
+			off = att_align_nominal(off, thisatt->attalign);
+
+			if (!slow)
+thisatt->attcacheoff = off;
+		}
+
+		off = att_addlength_pointer(off, thisatt->attlen, tp + off);
+
+		if (thisatt->attlen <= 0)
+			slow = true;		/* can't use attcacheoff anymore */
+
+		offsets[noffsets++] = off;
+	}
+
+	strategy = *PGLZ_strategy_always;
+	strategy.min_comp_rate = wal_update_compression_ratio;
+
+	return pglz_compress_with_history((char *) oldtup->t_data, oldtup->t_len,
+	  (char *) newtup->t_data, newtup->t_len,
+	  offsets, noffsets, (PGLZ_Header *) encdata,
+	  &strategy);
+}
+
+/* 
+ * heap_delta_decode
+ *
+ *		Decode a tuple using delta-encoded WAL tuple and old tuple version.
+ * 
+ */
+void
+heap_delta_decode(char *encdata, HeapTuple oldtup, HeapTuple newtup)
+{
+	return pglz_decompress_with_history((char *) encdata,
+		newtup->t_data,
+		&newtup->t_len,
+		(char *) oldtup->t_data,
+		oldtup->t_len);
+}
+
 /*
  * heap_form_tuple
  *		construct a tuple from the given values[] and isnull[] arrays,
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
ind

Re: [HACKERS] Re: proposal: a width specification for s specifier (format function), fix behave when positional and ordered placeholders are used

[Tom Lane]

Dean Rasheed  writes:
> On 28 January 2013 20:40, Pavel Stehule  wrote:
>> 2013/1/28 Dean Rasheed :
>>> flags - not currently implemented. Pavel's second patch adds support
>>> for the '-' flag for left justified string output. However, I think
>>> this should support all datatypes (i.e., %I and %L as well as %s).

>> no - surely not - I% and L% is PostgreSQL extension and left or right
>> alignment is has no sense for PostgreSQL identifiers and PostgreSQL
>> literals.

> Left/right alignment and padding in printf() apply to all types, after
> the data value is converted to a string. Why shouldn't that same
> principle apply to %I and %L?

I agree with Dean --- it would be very strange for these features not to
apply to all conversion specifiers (excepting %% of course, which isn't
really a conversion specifier but an escaping hack).

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] Re: proposal: a width specification for s specifier (format function), fix behave when positional and ordered placeholders are used

[Dean Rasheed]

On 28 January 2013 20:40, Pavel Stehule  wrote:
> 2013/1/28 Dean Rasheed :
>> On 28 January 2013 17:32, Stephen Frost  wrote:
>>> * Tom Lane (t...@sss.pgh.pa.us) wrote:
 Both.  If we had done this when we first implemented format(), it'd be
 fine, but it's too late to change it now.  There very likely are
 applications out there that depend on the current behavior.  As Dean
 says, it's not incompatible with SUS, just a superset, so ISTM this
 patch is proposing to remove documented functionality --- for no very
 strong reason.
>>>
>>> It's only a "superset" of the very poor subset of printf()-like
>>> functionality that we currently support through the format() function.
>>>
>>> If we can actually match glibc/SUS (which I don't believe the initial
>>> patch did..) and support a mix of explicitly specified arguments and
>>> implicit arguments, along with the various width, precision, and other
>>> format specifications, then fine by me.
>>>
>>> I'm not convinced that's actually possible due to the ambiguity which
>>> will certainly arise and I'm quite sure the documentation that explains
>>> what we do in each case will deserve it's own chapter.
>>>
>>
>> There are a number of separate issues here, but I don't see this as an
>> intractable problem. In general a format specifier looks like:
>>
>> %[parameter][flags][width][.precision][length]type
>>
>> parameter - an optional n$. This is where we have implemented a
>> superset of the SUS printf(). But I think it is a useful superset, and
>> it's too late to remove it now. Any ambiguity lies here, where we go
>> beyond the SUS - some printf() implementations appear to do something
>> different (apparently without documenting what they do). I think our
>> documentation could be clearer here, to explain how mixed parameters
>> are handled.
>>
>> flags - not currently implemented. Pavel's second patch adds support
>> for the '-' flag for left justified string output. However, I think
>> this should support all datatypes (i.e., %I and %L as well as %s).
>
> no - surely not - I% and L% is PostgreSQL extension and left or right
> alignment is has no sense for PostgreSQL identifiers and PostgreSQL
> literals.

Left/right alignment and padding in printf() apply to all types, after
the data value is converted to a string. Why shouldn't that same
principle apply to %I and %L? The obvious use-case is for producing
tabular output of data with columns neatly aligned. If we don't
support %I and %L then any alignment of columns to the right is lost.

Regards,
Dean


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: proposal: a width specification for s specifier (format function), fix behave when positional and ordered placeholders are used

[Pavel Stehule]

2013/1/28 Dean Rasheed :
> On 28 January 2013 17:32, Stephen Frost  wrote:
>> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>>> Both.  If we had done this when we first implemented format(), it'd be
>>> fine, but it's too late to change it now.  There very likely are
>>> applications out there that depend on the current behavior.  As Dean
>>> says, it's not incompatible with SUS, just a superset, so ISTM this
>>> patch is proposing to remove documented functionality --- for no very
>>> strong reason.
>>
>> It's only a "superset" of the very poor subset of printf()-like
>> functionality that we currently support through the format() function.
>>
>> If we can actually match glibc/SUS (which I don't believe the initial
>> patch did..) and support a mix of explicitly specified arguments and
>> implicit arguments, along with the various width, precision, and other
>> format specifications, then fine by me.
>>
>> I'm not convinced that's actually possible due to the ambiguity which
>> will certainly arise and I'm quite sure the documentation that explains
>> what we do in each case will deserve it's own chapter.
>>
>
> There are a number of separate issues here, but I don't see this as an
> intractable problem. In general a format specifier looks like:
>
> %[parameter][flags][width][.precision][length]type
>
> parameter - an optional n$. This is where we have implemented a
> superset of the SUS printf(). But I think it is a useful superset, and
> it's too late to remove it now. Any ambiguity lies here, where we go
> beyond the SUS - some printf() implementations appear to do something
> different (apparently without documenting what they do). I think our
> documentation could be clearer here, to explain how mixed parameters
> are handled.
>
> flags - not currently implemented. Pavel's second patch adds support
> for the '-' flag for left justified string output. However, I think
> this should support all datatypes (i.e., %I and %L as well as %s).

no - surely not - I% and L% is PostgreSQL extension and left or right
alignment is has no sense for PostgreSQL identifiers and PostgreSQL
literals.

Regards

Pavel

>
> width - not currently implemented. Pavel's second patch adds support
> for this, but note that for full compatibility with the SUS it needs
> to also support widths specified using * and *n$. Also, I think it
> should support all supported datatypes, not just strings.
>
> precision - only relevant to numeric datatypes, which we don't support.
>
> length - only relevant to numeric datatypes, which we don't support.
>
> type - this is where we only support a small subset of the SUS (plus a
> couple of SQL-specific types). I'm not sure if anyone has any plans to
> extend this, but that's certainly not on the cards for 9.3.
>
>
> So the relevant pieces that Pavel's second patch is attempting to add
> support for are the '-' flag and the width field. As noted above,
> there are a couple of areas where the current patch falls short of the
> SUS:
>
> 1). The '-' flag and width field are supposed to apply to all types. I
> think that not supporting %I and %L will be somewhat limiting, and
> goes against the intent of the SUS, even though those types are
> PostgreSQL extensions.
>
> 2). The width field is supposed to support * (width specified by the
> next function argument) and *n$ (width specified by the nth function
> argument). If we supported this, then we could claim full
> compatibility with the SUS in all fields except for the type support,
> which would seem like a real step forward.
>
> Regards,
> Dean


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: proposal: a width specification for s specifier (format function), fix behave when positional and ordered placeholders are used

[Dean Rasheed]

On 28 January 2013 17:32, Stephen Frost  wrote:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> Both.  If we had done this when we first implemented format(), it'd be
>> fine, but it's too late to change it now.  There very likely are
>> applications out there that depend on the current behavior.  As Dean
>> says, it's not incompatible with SUS, just a superset, so ISTM this
>> patch is proposing to remove documented functionality --- for no very
>> strong reason.
>
> It's only a "superset" of the very poor subset of printf()-like
> functionality that we currently support through the format() function.
>
> If we can actually match glibc/SUS (which I don't believe the initial
> patch did..) and support a mix of explicitly specified arguments and
> implicit arguments, along with the various width, precision, and other
> format specifications, then fine by me.
>
> I'm not convinced that's actually possible due to the ambiguity which
> will certainly arise and I'm quite sure the documentation that explains
> what we do in each case will deserve it's own chapter.
>

There are a number of separate issues here, but I don't see this as an
intractable problem. In general a format specifier looks like:

%[parameter][flags][width][.precision][length]type

parameter - an optional n$. This is where we have implemented a
superset of the SUS printf(). But I think it is a useful superset, and
it's too late to remove it now. Any ambiguity lies here, where we go
beyond the SUS - some printf() implementations appear to do something
different (apparently without documenting what they do). I think our
documentation could be clearer here, to explain how mixed parameters
are handled.

flags - not currently implemented. Pavel's second patch adds support
for the '-' flag for left justified string output. However, I think
this should support all datatypes (i.e., %I and %L as well as %s).

width - not currently implemented. Pavel's second patch adds support
for this, but note that for full compatibility with the SUS it needs
to also support widths specified using * and *n$. Also, I think it
should support all supported datatypes, not just strings.

precision - only relevant to numeric datatypes, which we don't support.

length - only relevant to numeric datatypes, which we don't support.

type - this is where we only support a small subset of the SUS (plus a
couple of SQL-specific types). I'm not sure if anyone has any plans to
extend this, but that's certainly not on the cards for 9.3.


So the relevant pieces that Pavel's second patch is attempting to add
support for are the '-' flag and the width field. As noted above,
there are a couple of areas where the current patch falls short of the
SUS:

1). The '-' flag and width field are supposed to apply to all types. I
think that not supporting %I and %L will be somewhat limiting, and
goes against the intent of the SUS, even though those types are
PostgreSQL extensions.

2). The width field is supposed to support * (width specified by the
next function argument) and *n$ (width specified by the nth function
argument). If we supported this, then we could claim full
compatibility with the SUS in all fields except for the type support,
which would seem like a real step forward.

Regards,
Dean


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] enhanced error fields

[Pavel Stehule]

2013/1/28 Tom Lane :
> Pavel Stehule  writes:
>> 2013/1/28 Tom Lane :
>>> ...  The current patch provides sufficient
>>> information to uniquely identify a table constraint, but not so much
>>> domain constraints.  Should we fix that?  I think it'd be legitimate
>>> to re-use SCHEMA_NAME for domain schema, but we'd need a new nonstandard
>>> field DOMAIN_NAME (or maybe better DATATYPE_NAME) if we want to fix it.
>>> Do we want to add that now?
>
>> should be for me.
>
>> one question - what do you thing about marking proprietary field with
>> some prefix - like PG_DOMAIN_NAME ?
>
> Don't particularly see the point of that.  It seems quite unlikely that
> the ISO committee would invent a field with the same name and a
> conflicting definition.  Anyway, these names aren't going to be exposed
> in any non "proprietary" interfaces AFAICS.  Surely we don't, for
> instance, need to call the postgres_ext.h macro PG_DIAG_PG_DOMAIN_NAME.

ok

Pavel

>
> 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] enhanced error fields

[Tom Lane]

Pavel Stehule  writes:
> 2013/1/28 Tom Lane :
>> ...  The current patch provides sufficient
>> information to uniquely identify a table constraint, but not so much
>> domain constraints.  Should we fix that?  I think it'd be legitimate
>> to re-use SCHEMA_NAME for domain schema, but we'd need a new nonstandard
>> field DOMAIN_NAME (or maybe better DATATYPE_NAME) if we want to fix it.
>> Do we want to add that now?

> should be for me.

> one question - what do you thing about marking proprietary field with
> some prefix - like PG_DOMAIN_NAME ?

Don't particularly see the point of that.  It seems quite unlikely that
the ISO committee would invent a field with the same name and a
conflicting definition.  Anyway, these names aren't going to be exposed
in any non "proprietary" interfaces AFAICS.  Surely we don't, for
instance, need to call the postgres_ext.h macro PG_DIAG_PG_DOMAIN_NAME.

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 - assign result of query to psql variable

[Pavel Stehule]

Hello

2013/1/26 Tom Lane :
> Andrew Dunstan  writes:
>> +1. This looks quite nifty. Maybe useful too to have a default prefix
>> via some setting.
>
> Meh.  I would expect that "\gset :foo" would work to specify a computed
> prefix if you wanted it --- isn't that sufficient indirection?  I'm not
> thrilled with further expanding the set of magic variables in psql.
>

here is patch related to your proposal

Regards

Pavel

> regards, tom lane


gset_20130128.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] enhanced error fields

[Pavel Stehule]

2013/1/28 Tom Lane :
> A couple more things about this patch ...
>
> I went back through the thread and reviewed all the angst about which
> fields to provide, especially whether we need CONSTRAINT_SCHEMA.
> I agree with the conclusion that we don't.  It's in the spec because
> the spec supposes that CONSTRAINT_SCHEMA+CONSTRAINT_NAME is a unique
> identification for a constraint --- but it is not in Postgres, for
> historical reasons that we aren't going to revisit in this patch.
> Rather what we've got is that constraints are uniquely named among
> those associated with a table, or with a domain.  So the correct
> unique key for a table constraint is table schema + table name +
> constraint name, whereas for a domain constraint it's domain schema +
> domain name + constraint name.  The current patch provides sufficient
> information to uniquely identify a table constraint, but not so much
> domain constraints.  Should we fix that?  I think it'd be legitimate
> to re-use SCHEMA_NAME for domain schema, but we'd need a new nonstandard
> field DOMAIN_NAME (or maybe better DATATYPE_NAME) if we want to fix it.
> Do we want to add that now?

should be for me.

one question - what do you thing about marking proprietary field with
some prefix - like PG_DOMAIN_NAME ?

>
> If we do fix this, either now or later, it's going to require some small
> support function very much like errtable() to perform the catalog
> lookups for a datatype and set the ErrorData fields.  The thought of
> dropping such a thing into relerror.c exposes the fact that that
> "module" isn't actually very modular.  We could choose a different name
> for the file but it'd still be pretty questionable as to what its
> boundaries are.  So I'm a bit inclined to drop relerror.c as such, and
> go back to the early design that put errtable() and friends in
> relcache.c.  The errconstraint() function, not being specific to either
> rels or datatypes, perhaps really does belong in elog.c.
>

+1

Pavel

> Thoughts?
>
> regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] SYSV shared memory vs mmap performance

[Francois Tigeot]

Hi,

On Mon, Jan 28, 2013 at 03:27:28PM +, YAMAMOTO Takashi wrote:
> 
> can you give me a pointer?

This bug report for a start:
http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=46833

This is the only one I've filled; I also remember having irc discussions
with some netbsd developers about the system getting unresponsive under
load.

-- 
Francois Tigeot


-- 
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] SYSV shared memory vs mmap performance

[YAMAMOTO Takashi]

hi,

> I'm less optimistic on the NetBSD front: even though I reported major
> show-stopper bugs (system died under load and was unable to complete
> a pgbench run), no one seemed to care.

can you give me a pointer?

YAMAMOTO Takashi


-- 
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] in-catalog Extension Scripts and Control parameters (templates?)

[Tom Lane]

Dimitri Fontaine  writes:
> Now that I've written this in that email, I think I'm going to go for
> the new command. But maybe we have some precedent for objects that we
> list in pg_dump only for solving several steps dependency lookups?

Yes, pg_dump has lots of objects that might not appear in a dump.
The most recent examples are the section fence objects ...

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] pg_isready (was: [WIP] pg_ping utility)

[Phil Sorber]

On Mon, Jan 28, 2013 at 1:12 PM, Alvaro Herrera
 wrote:
> Phil Sorber escribió:
>> On Mon, Jan 28, 2013 at 11:20 AM, Fujii Masao  wrote:
>> > Maybe. But I'm not inclined to add new libpq interface at this stage.
>> > Because we are in the last CommitFest and I'm not sure whether
>> > we have enough time to implement that. Instead, how about using
>> > both PQconninfoParse() and PQconndefaults() like uri-regress.c does?
>> > Or just remove that output message? At least I don't think that the
>> > information about host and port needs to be output. Which would make
>> > the code very simpler.
>>
>> I think that output is important as do others up thread. I think it'd
>> be simpler to just disable dbname's ability to double as conninfo. If
>> we are looking for easy, that is.
>>
>> I don't mind duplicating the behavior from conninfo_array_parse() or
>> uri-regress.c either. I can just put a comment that at some point in
>> the future we should add a libpq interface for it.
>
> I suggest duplicate the code for 9.3, and submit a patch to refactor
> into a new libpq function for CF2013-next.  If the patch is simple
> enough, we can consider putting it into 9.3.

Ok, sounds good to me.

>
> --
> Á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] [PATCH] pg_isready (was: [WIP] pg_ping utility)

[Alvaro Herrera]

Phil Sorber escribió:
> On Mon, Jan 28, 2013 at 11:20 AM, Fujii Masao  wrote:
> > Maybe. But I'm not inclined to add new libpq interface at this stage.
> > Because we are in the last CommitFest and I'm not sure whether
> > we have enough time to implement that. Instead, how about using
> > both PQconninfoParse() and PQconndefaults() like uri-regress.c does?
> > Or just remove that output message? At least I don't think that the
> > information about host and port needs to be output. Which would make
> > the code very simpler.
> 
> I think that output is important as do others up thread. I think it'd
> be simpler to just disable dbname's ability to double as conninfo. If
> we are looking for easy, that is.
> 
> I don't mind duplicating the behavior from conninfo_array_parse() or
> uri-regress.c either. I can just put a comment that at some point in
> the future we should add a libpq interface for it.

I suggest duplicate the code for 9.3, and submit a patch to refactor
into a new libpq function for CF2013-next.  If the patch is simple
enough, we can consider putting it into 9.3.

-- 
Á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] enhanced error fields

[Tom Lane]

A couple more things about this patch ...

I went back through the thread and reviewed all the angst about which
fields to provide, especially whether we need CONSTRAINT_SCHEMA.
I agree with the conclusion that we don't.  It's in the spec because
the spec supposes that CONSTRAINT_SCHEMA+CONSTRAINT_NAME is a unique
identification for a constraint --- but it is not in Postgres, for
historical reasons that we aren't going to revisit in this patch.
Rather what we've got is that constraints are uniquely named among
those associated with a table, or with a domain.  So the correct
unique key for a table constraint is table schema + table name +
constraint name, whereas for a domain constraint it's domain schema +
domain name + constraint name.  The current patch provides sufficient
information to uniquely identify a table constraint, but not so much
domain constraints.  Should we fix that?  I think it'd be legitimate
to re-use SCHEMA_NAME for domain schema, but we'd need a new nonstandard
field DOMAIN_NAME (or maybe better DATATYPE_NAME) if we want to fix it.
Do we want to add that now?

If we do fix this, either now or later, it's going to require some small
support function very much like errtable() to perform the catalog
lookups for a datatype and set the ErrorData fields.  The thought of
dropping such a thing into relerror.c exposes the fact that that
"module" isn't actually very modular.  We could choose a different name
for the file but it'd still be pretty questionable as to what its
boundaries are.  So I'm a bit inclined to drop relerror.c as such, and
go back to the early design that put errtable() and friends in
relcache.c.  The errconstraint() function, not being specific to either
rels or datatypes, perhaps really does belong in elog.c.

Thoughts?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: index support for regexp search

[Alexander Korotkov]

On Sun, Jan 27, 2013 at 10:40 PM, Alexander Korotkov
wrote:

> Now I'm working on additional comments.


Some comments were added for addKey and addArc(s). I hope they clarify
something.

--
With best regards,
Alexander Korotkov.


trgm-regexp-0.12.patch.gz
Description: GNU Zip compressed data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: proposal: a width specification for s specifier (format function), fix behave when positional and ordered placeholders are used

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Both.  If we had done this when we first implemented format(), it'd be
> fine, but it's too late to change it now.  There very likely are
> applications out there that depend on the current behavior.  As Dean
> says, it's not incompatible with SUS, just a superset, so ISTM this
> patch is proposing to remove documented functionality --- for no very
> strong reason.

It's only a "superset" of the very poor subset of printf()-like
functionality that we currently support through the format() function.

If we can actually match glibc/SUS (which I don't believe the initial
patch did..) and support a mix of explicitly specified arguments and
implicit arguments, along with the various width, precision, and other
format specifications, then fine by me.

I'm not convinced that's actually possible due to the ambiguity which
will certainly arise and I'm quite sure the documentation that explains
what we do in each case will deserve it's own chapter.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] in-catalog Extension Scripts and Control parameters (templates?)

[Dimitri Fontaine]

Dimitri Fontaine  writes:
> Please find attached a new version of the patch, answering to most of
> your reviewing points. I'll post another version shortly with support
> for pg_dump and alter owner/rename.

So, as far as pg_dump is concerned, I have a trick question here.

We now have those new catalogs:

  - pg_extension_control
  - pg_extension_template
  - pg_extension_uptmpl

When doing CREATE EXTENSION, if we did use a template to find the
control information about it, we record a dependency. The template and
update_template (named uptmpl to make the name shorter) catalog entries
also have a pg_depend dependency towards the pg_extension_control.

Now, at pg_dump time, I want to be dumping the templates for the
extension and for updating the extension *before* the extension itself.
It seems to me that the dependency as setup will guarantee that.

The trick is that I don't have anything to dump for a given control
entry itself. So I could either add some more commands so that pg_dump
can setup the control then the template for creating or updating an
extension, or just have a dumpExtensionControl() that does nothing.

I'm not sure about which one to pick. Did I explain the problem properly
enough for someone to chime in?

Now that I've written this in that email, I think I'm going to go for
the new command. But maybe we have some precedent for objects that we
list in pg_dump only for solving several steps dependency lookups?

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

Re: [HACKERS] Number of buckets in a hash join

[Tom Lane]

Heikki Linnakangas  writes:
> The first question is, why do we aim at 10 tuples per bucket?

I see nothing particularly wrong with that.  The problem here is with
having 1000 tuples per bucket.

> Ideally, the planner would always make a good guess the number of rows, 
> but for the situations that it doesn't, it would be good if the hash 
> table was enlarged if it becomes too full.

Yeah, possibly.  The proposed test case actually doesn't behave very
badly if work_mem is small, because there is logic in there to adjust
the number of batches.  You didn't say what work_mem you're testing at,
but it's clearly more than the default 1MB.  I think the issue arises if
the initial estimate of hashtable size is a good bit less than work_mem,
so the number of buckets is set to something a good bit less than what
would be optimal if we're using more of work_mem.  This seems a little
reminiscent of what we did recently in tuplesort to make better use of
work_mem --- in both cases we have to choose a pointer-array size that
will make best use of work_mem after the tuples themselves are added.

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] pg_isready (was: [WIP] pg_ping utility)

[Phil Sorber]

On Mon, Jan 28, 2013 at 11:20 AM, Fujii Masao  wrote:
> Maybe. But I'm not inclined to add new libpq interface at this stage.
> Because we are in the last CommitFest and I'm not sure whether
> we have enough time to implement that. Instead, how about using
> both PQconninfoParse() and PQconndefaults() like uri-regress.c does?
> Or just remove that output message? At least I don't think that the
> information about host and port needs to be output. Which would make
> the code very simpler.
>

I think that output is important as do others up thread. I think it'd
be simpler to just disable dbname's ability to double as conninfo. If
we are looking for easy, that is.

I don't mind duplicating the behavior from conninfo_array_parse() or
uri-regress.c either. I can just put a comment that at some point in
the future we should add a libpq interface for it.

> Regards,
>
> --
> Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] autovacuum not prioritising for-wraparound tables

[Andres Freund]

On 2013-01-28 08:15:29 -0800, Kevin Grittner wrote:
> Andres Freund  wrote:
> > On 2013-01-29 00:11:12 +1100, Josh Berkus wrote:
> >>
> >>> So I think we need to sort by age(relfrozenxid) in tables that
> >>> are over the anti-wraparound limit. Given your code that
> >>> doesn't seem to be that hard?
> >>
> >> I might also suggest that we think about changing the defaults
> >> for wraparound vacuum behavior.  Partcularly, the fact that
> >> vacuum_freeze_min_age is 50% of autovacuum_freeze_max_age by
> >> default is optimal for absolutely nobody, and forces
> >> re-wraparound vacuuming of wraparound tables which were just
> >> recently wraparound-vacuumed. We should lower
> >> vacuum_freeze_min_age to something sane, like 100.
> >
> > I have to admit, I fail to see why this is a good idea. There
> > isn't much of an efficiency bonus in freezing early (due to hint
> > bits) and vacuums over vacuum_freeze_table_age are considerably
> > more expensive as they have to scan the whole heap instead of
> > using the visibilitymap. And if you don't vacuum the whole heap
> > you can't lower relfrozenxid. So changing freeze_min_age doesn't
> > help at all to avoid anti-wraparound vacuums.
> >
> > Am I missing something?
> 
> IMO, anything which changes an anti-wraparound vacuum of a
> bulk-loaded table from "read the entire table and rewrite nearly
> the complete table with WAL-logging" to rewriting a smaller portion
> of the table with WAL-logging is an improvement.

Yes, but the proposed changes make that *more* frequent, not less. 1mio
transactions will usually be *after* the next checkpoint and thus be
written twice.

Imnsho the solution to actually solve that problem is to have have
'freeze map' that marks blocks (say 16MB) of tables as already frozen so
it doesn't have to be reread/written every by (auto-)vacuum at all.
I would like to work on that sometime in the future, but it won't be all
that soon...

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] Re: proposal: a width specification for s specifier (format function), fix behave when positional and ordered placeholders are used

[Tom Lane]

Pavel Stehule  writes:
> 2013/1/28 Dean Rasheed :
>> Starting with the first patch - it issues a new WARNING if the format
>> string contains a mixture of format specifiers with and without
>> parameter indexes (e.g., 'Hello %s, %1$s').
>> 
>> Having thought about it a bit, I really don't like this for a number of 
>> reasons:

> I am not sure what you dislike?
> warnings or redesign of behave.

Both.  If we had done this when we first implemented format(), it'd be
fine, but it's too late to change it now.  There very likely are
applications out there that depend on the current behavior.  As Dean
says, it's not incompatible with SUS, just a superset, so ISTM this
patch is proposing to remove documented functionality --- for no very
strong reason.

I vote for rejecting this change entirely.

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] pg_isready (was: [WIP] pg_ping utility)

[Fujii Masao]

On Mon, Jan 28, 2013 at 4:47 AM, Phil Sorber  wrote:
> On Sun, Jan 27, 2013 at 2:38 PM, Phil Sorber  wrote:
>> On Fri, Jan 25, 2013 at 11:20 AM, Fujii Masao  wrote:
>>> On Fri, Jan 25, 2013 at 4:10 AM, Phil Sorber  wrote:
 On Thu, Jan 24, 2013 at 1:12 PM, Fujii Masao  wrote:
> set_pglocale_pgservice() should be called?
>
> I think that the command name (i.e., pg_isready) should be given to
> PQpingParams() as fallback_application_name. Otherwise, the server
> by default uses "unknown" as the application name of pg_isready.
> It's undesirable.
>
> Why isn't the following message output only when invalid option is
> specified?
>
> Try \"%s --help\" for more information.

 I've updated the patch to address these three issues. Attached.

>
> When the conninfo string including the hostname or port number is
> specified in -d option, pg_isready displays the wrong information
> as follows.
>
> $ pg_isready -d "port="
> /tmp:5432 - no response
>

 This is what i asked about in my previous email about precedence of
 the parameters. I can parse that with PQconninfoParse, but what are
 the rules for merging both individual and conninfo params together?
>>>
>>> If I read conninfo_array_parse() correctly, PQpingParams() prefer the
>>> option which is set to its keyword array later.
>>
>> It would be really nice to expose conninfo_array_parse() or some
>> wrapped version directly to a libpq consumer. Otherwise, I need to
>> recreate this behavior in pg_isready.c.
>>
>> Thoughts on adding:
>>   PQconninfoOption *PQparamsParse(const char **keywords, const char
>> **values, char **errmsg, bool use_defaults, int expand_dbname)
>> or similar?

Maybe. But I'm not inclined to add new libpq interface at this stage.
Because we are in the last CommitFest and I'm not sure whether
we have enough time to implement that. Instead, how about using
both PQconninfoParse() and PQconndefaults() like uri-regress.c does?
Or just remove that output message? At least I don't think that the
information about host and port needs to be output. Which would make
the code very simpler.

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] autovacuum not prioritising for-wraparound tables

[Kevin Grittner]

Andres Freund  wrote:
> On 2013-01-29 00:11:12 +1100, Josh Berkus wrote:
>>
>>> So I think we need to sort by age(relfrozenxid) in tables that
>>> are over the anti-wraparound limit. Given your code that
>>> doesn't seem to be that hard?
>>
>> I might also suggest that we think about changing the defaults
>> for wraparound vacuum behavior.  Partcularly, the fact that
>> vacuum_freeze_min_age is 50% of autovacuum_freeze_max_age by
>> default is optimal for absolutely nobody, and forces
>> re-wraparound vacuuming of wraparound tables which were just
>> recently wraparound-vacuumed. We should lower
>> vacuum_freeze_min_age to something sane, like 100.
>
> I have to admit, I fail to see why this is a good idea. There
> isn't much of an efficiency bonus in freezing early (due to hint
> bits) and vacuums over vacuum_freeze_table_age are considerably
> more expensive as they have to scan the whole heap instead of
> using the visibilitymap. And if you don't vacuum the whole heap
> you can't lower relfrozenxid. So changing freeze_min_age doesn't
> help at all to avoid anti-wraparound vacuums.
>
> Am I missing something?

IMO, anything which changes an anti-wraparound vacuum of a
bulk-loaded table from "read the entire table and rewrite nearly
the complete table with WAL-logging" to rewriting a smaller portion
of the table with WAL-logging is an improvement.  Anyone who has
run an OLTP load on a database which was loaded from pg_dump output
or other bulk load processes, has probably experienced the pain
related to the WAL-logged rewrite of massive quantities of data. 
Of course, since it triggers based on transaction count, the higher
your transaction rate at any moment, the more likely it is to kick
in at that moment.  Whatever we can do to reduce that pain helps.

-Kevin


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: proposal: a width specification for s specifier (format function), fix behave when positional and ordered placeholders are used

[Pavel Stehule]

Hello

2013/1/28 Dean Rasheed :
> On 26 January 2013 10:58, Pavel Stehule  wrote:
>> updated patches due changes for better variadic "any" function.
>>
>> apply fix_mixing_positinal_ordered_placeholders_warnings_20130126.patch first
>>
>
> Hi,
>
> No one is listed as a reviewer for this patch so I thought I would
> take a look at it, since it looks like a useful enhancement to
> format().
>
> Starting with the first patch - it issues a new WARNING if the format
> string contains a mixture of format specifiers with and without
> parameter indexes (e.g., 'Hello %s, %1$s').
>
> Having thought about it a bit, I really don't like this for a number of 
> reasons:
>
> * I actually quite like the current behaviour. Admittedly putting
> ordered specifiers (like '%s') after positional ones (like '%3$s') is
> probably not so useful, and potentially open to different
> interpretations. But putting positional specifiers at the end is
> completely unambiguous and can save a lot of typing (e.g.,
> '%s,%s,%s,%s,%,s,%s,%s,%1$s').
>
> * On backwards compatibility grounds. The fact that the only example
> of format() in the manual is precisely a case of mixed positional and
> ordered parameters makes it quite likely that people will have used
> this feature already.
>
> * Part of the justification for adding the warning is for
> compatibility with glibc/SUS printf(). But if we are aiming for that,
> then we should also produce a warning if positional parameters are
> used and not all parameters are consumed. That would be a pain to
> implement and I don't think it would be particularly helpful in
> practice. Here is what the SUS says:
>
> """
> The format can contain either numbered argument specifications (that
> is, %n$ and *m$), or unnumbered argument specifications (that is, %
> and *), but normally not both. The only exception to this is that %%
> can be mixed with the %n$ form. The results of mixing numbered and
> unnumbered argument specifications in a format string are undefined.
> When numbered argument specifications are used, specifying the Nth
> argument requires that all the leading arguments, from the first to
> the (N-1)th, are specified in the format string.
> """
>
> I think that if we are going to do anything, we should explicitly
> document our current behaviour as a PostgreSQL extension to the SUS
> printf(), describing how we handle mixed parameters, rather than
> adding this warning.
>
> The current PostgreSQL code isn't inconsistent with the SUS, except in
> the error case, and so can reasonably be regarded as an extension.
>

I am not sure what you dislike?

warnings or redesign of behave.

I can live without warnings, when this field will be documented - it
is not fully compatible with gcc, but gcc just raises warnings and
does correct implementation. Our warnings are on different level than
gcc warnings.

But I don't think so current implementation is correct

-- our current behave
postgres=# select format('%s %2$s %s', 'Hello', 'World');
ERROR:  too few arguments for format
postgres=#

postgres=# select format('%s %1$s %s', 'Hello', 'World'); -- works

ordered parameters should be independent on positional parameters. And
this behave has glibc

Regards

Pavel

> Regards,
> Dean


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_ctl idempotent option

[Simon Riggs]

On 14 January 2013 15:29, Alvaro Herrera  wrote:
> Tom Lane wrote:
>> Peter Eisentraut  writes:
>> > Here is a patch to add an option -I/--idempotent to pg_ctl, the result
>> > of which is that pg_ctl doesn't error on start or stop if the server is
>> > already running or already stopped.
>>
>> Idempotent is a ten-dollar word.  Can we find something that average
>> people wouldn't need to consult a dictionary to understand?
>
> --no-error perhaps?


I think --force  would be the accepted way to ensure something happens
as specified


Mind you, I'm not sure I see the value in throwing an error if the
server is in the desired state already. Who actually wants that
behaviour? Can't we just change the behaviour? Existing scripts would
still work, since we are simply skipping an error.

-- 
 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] plpgsql_check_function - rebase for 9.3

[Andrew Dunstan]

On 01/28/2013 10:11 AM, Peter Eisentraut wrote:

On 1/26/13 1:53 PM, Tom Lane wrote:

[ pokes around... ]  Hm, it appears that that does work on Linux,
because for some reason we're specifying RTLD_GLOBAL to dlopen().
TBH that seems like a truly horrid idea that we should reconsider.
Aside from the danger of unexpected symbol collisions between
independent loadable modules, I seriously doubt that it works like
that on every platform we support --- so I'd be very strongly against
accepting any code that depends on this working.

Well, that would kill a lot of potentially useful features, including
the transforms feature I've been working on and any kind of hook or
debugger or profiler on an existing module.  (How do plpgsql plugins
work?)  We also couldn't transparently move functionality out of the
postgres binary into a module.

I see the concern about symbol collisions.  But you can normally work
around that by prefixing exported symbols.



Yeah, I was just writing something a couple of days ago that leveraged 
stuff in an extension, so it looks like this is wanted functionality. In 
general we want to be able to layer addon modules, ISTM.


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] pg_ctl idempotent option

[Asif Naeem]

I am working on reviewing the patch. Patch apply without warning/error on
master branch. My findings are as following i.e.

1. Behavior change in pg_ctl return value i.e.
*
*
* Server already running*

a. Without Patch

inst asif$ ./bin/pg_ctl -D data_test/ -l data_test.log start

pg_ctl: another server might be running; trying to start server anyway

server starting

inst asif$ echo $?

0


b. With Patch


inst_pg_ctl_idempotent_option asif$ ./bin/pg_ctl -D data_test/ -l
> data_test.log start
> pg_ctl: another server might be running
> inst_pg_ctl_idempotent_option asif$ echo $?
> 1


2. -w option seems not working for start as per documentation, it should
return 0.

*Starting already running server with -I -w option*


inst_pg_ctl_idempotent_option asif$ ./bin/pg_ctl -D data_test/ -l
> data_test.log -I -w start

pg_ctl: another server might be running; trying to start server anyway

waiting for server to start

pg_ctl: this data directory appears to be running a pre-existing postmaster

 stopped waiting

pg_ctl: could not start server

Examine the log output.

inst_pg_ctl_idempotent_option asif$ echo $?

1


3. I believe postmaster (DAEMON="$prefix/bin/postmaster") is not going to
accept "-I" option as mentioned in the patch i.e.

contrib/start-scripts/linux

>  su - $PGUSER -c "$DAEMON -I -D '$PGDATA' &" >>$PGLOG 2>&


Rest of the patch changes looks good to me. Thanks.

Best Regards,
Asif Naeem

On Thu, Jan 24, 2013 at 6:06 PM, Bruce Momjian  wrote:

> On Thu, Jan 24, 2013 at 09:05:59AM +0530, Ashutosh Bapat wrote:
> > I agree, answering the question, whether the particular attempt of
> > starting a server succeeded or not, will need the current behaviour.
> > Now, question is which of these behaviours should be default?
>
> That would work.  pg_upgrade knows the cluster version at that point and
> can use the proper flag.
>
> --
>   Bruce Momjian  http://momjian.us
>   EnterpriseDB http://enterprisedb.com
>
>   + It's impossible for everything to be true. +
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

Re: [HACKERS] plpgsql_check_function - rebase for 9.3

[Peter Eisentraut]

On 1/26/13 1:53 PM, Tom Lane wrote:
> [ pokes around... ]  Hm, it appears that that does work on Linux,
> because for some reason we're specifying RTLD_GLOBAL to dlopen().
> TBH that seems like a truly horrid idea that we should reconsider.
> Aside from the danger of unexpected symbol collisions between
> independent loadable modules, I seriously doubt that it works like
> that on every platform we support --- so I'd be very strongly against
> accepting any code that depends on this working.

Well, that would kill a lot of potentially useful features, including
the transforms feature I've been working on and any kind of hook or
debugger or profiler on an existing module.  (How do plpgsql plugins
work?)  We also couldn't transparently move functionality out of the
postgres binary into a module.

I see the concern about symbol collisions.  But you can normally work
around that by prefixing exported symbols.


-- 
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_ctl promote" exit status

[Tom Lane]

Kevin Grittner  writes:
> Heikki Linnakangas  wrote:
>> Not sure if that LSB section is relevant anyway. It specifies the
>> exit codes for init scripts, but pg_ctl is not an init script.

> Except that when I went to the trouble of wrapping pg_ctl with an
> init script which was thoroughly LSB compliant (according to my
> reading) and offered it to the community, everyone said that rather
> than have such a complicated script it would be better to change
> pg_ctl to include that logic and exit with an LSB compliant exit
> code.

Right.  The start and stop actions are commonly used in initscripts
so it'd be handy if the exit codes for those didn't need to be
remapped.

On the other hand, it's not at all clear to me that anyone would try
to put the promote action into an initscript, or that LSB would have
anything to say about the exit codes for such a nonstandard action
anyway.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] "pg_ctl promote" exit status

[Peter Eisentraut]

On 1/26/13 4:44 PM, Aaron W. Swenson wrote:
> You are right. Had I read a little further down, it seems that the
> exit status should actually be 7.

7 is OK for "not running", but what should we use when the server is not
in standby mode?  Using the idempotent argument that we are discussing
for the stop action, promoting a server that is not a standby should be
a noop and exit successfully.  Not sure if that is what we want, though.



-- 
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] Number of buckets in a hash join

[Simon Riggs]

On 28 January 2013 10:47, Heikki Linnakangas  wrote:

> There's also some overhead from empty
> buckets when scanning the hash table

Seems like we should measure that overhead. That way we can plot the
cost against number per bucket, which sounds like it has a minima at
1.0, but that doesn't mean its symmetrical about that point. We can
then see where the optimal setting should be.

Having said that the hash bucket estimate is based on ndistinct, which
we know is frequently under-estimated, so it would be useful to err on
the low side.

-- 
 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] "pg_ctl promote" exit status

[Kevin Grittner]

Heikki Linnakangas  wrote:

> Not sure if that LSB section is relevant anyway. It specifies the
> exit codes for init scripts, but pg_ctl is not an init script.

Except that when I went to the trouble of wrapping pg_ctl with an
init script which was thoroughly LSB compliant (according to my
reading) and offered it to the community, everyone said that rather
than have such a complicated script it would be better to change
pg_ctl to include that logic and exit with an LSB compliant exit
code.

-Kevin


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for REINDEX CONCURRENTLY

[Michael Paquier]

On Mon, Jan 28, 2013 at 8:59 PM, Andres Freund wrote:

> On 2013-01-28 20:50:21 +0900, Michael Paquier wrote:
> > On Mon, Jan 28, 2013 at 8:44 PM, Andres Freund 
> wrote:
> >
> > > > Another argument that would be enough for a rejection of this patch
> by a
> > > > committer is the problem of invalid toast indexes that cannot be
> removed
> > > up
> > > > cleanly by an operator. As long as there is not a clean solution for
> > > > that...
> > >
> > > I think that part is relatively easy to fix, I wouldn't worry too
> > > much.
> > > The more complex part is how to get tuptoaster.c to update the
> > > concurrently created index. That's what I worry about. Its not going
> > > through the normal executor paths but manually updates the toast
> > > index - which means it won't update the indisready && !indisvalid
> > > index...
> > >
> > I included in the patch some stuff to update the reltoastidxid of the
> > parent relation of the toast index. Have a look at
> > index.c:index_concurrent_swap. The particular case I had in mind was if
> > there is a failure of the server during the concurrent reindex of a toast
> > index.
>
> Thats not enough unfortunately. The problem scenario is the following:
>
> toast table: pg_toast.pg_toast_16384
> toast index (via reltoastidxid): pg_toast.pg_toast_16384_index
> REINDEX CONCURRENTLY PHASE #1
> REINDEX CONCURRENTLY PHASE #2
> toast table: pg_toast.pg_toast_16384
> toast index (via reltoastidxid): pg_toast.pg_toast_16384_index, ready &
> valid
> toast index (via pg_index): pg_toast.pg_toast_16384_index_tmp, ready &
> !valid
>
> If a tuple gets toasted in this state tuptoaster.c will update
> 16384_index but not 16384_index_tmp. In normal tables this works because
> nodeModifyTable uses ExecInsertIndexTuples which updates all ready
> indexes. tuptoaster.c does something different though, it calls
> index_insert exactly on the one expected index, not on the other ones.
>
> Makes sense?
>
I didn't know toast indexes followed this code path. Thanks for the
details.

>
> > When server restarts, the toast relation will have an invalid index
> > and this cannot be dropped by an operator via SQL.
>
> That requires about two lines of special case code in
> RangeVarCallbackForDropRelation, that doesn't seem to be too bad to me.
>
> I.e. allow the case where its IsSystemClass(classform) && relkind ==
> RELKIND_INDEX && !indisvalid.
>
OK, I thought it was more complicated.
-- 
Michael Paquier
http://michael.otacoo.com

Re: [HACKERS] pg_catalog

[Graham Little]

Fabrizio,

Thank you very much for your email, I was able to run a script to generate 
update statements to the tables you mentioned and this has fixed my problem for 
me.

select'UPDATE pg_catalog.pg_class SET reltriggers=' || b.reltriggers-1 || ' 
WHERE relname=' ||  || a.table_name ||  || ' and oid=' || b.oid
from  information_schema.tables a, pg_catalog.pg_class b
where   a.table_type = 'BASE TABLE'
anda.table_schema = 'public'
anda.table_name not like '%dbmirror%'
anda.table_name = b.relname
order by a.table_name asc

Many Thanks

Graham

From: Fabrízio de Royes Mello [mailto:fabriziome...@gmail.com]
Sent: 28 January 2013 15:03
To: Graham Little
Cc: pgsql-hackers@postgresql.org
Subject: Re: [HACKERS] pg_catalog


On Mon, Jan 28, 2013 at 10:24 AM, Graham Little 
mailto:graham.lit...@aspone.co.uk>> wrote:
>
> Hi,
>
>
>
> I have tried other sources but to no avail. Could someone please tell me 
> which tables in pg_catalog
> are effected by creating or dropping a trigger. If there is a work flow 
> diagram or source code location
> you want to point me to rather than listing them that will be fine also.

I don't know if its completely right, but when a trigger is created the 
following catalog are affecteds:
- pg_trigger (new entry)
- pg_depend (new entry)
- pg_class (update relhastriggers column)


> Some plonker deleted some rows directly in the pg_triggers table  I need 
> to fix it.

Maybe you simply DROP and CREATE trigger solve it...

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello

Re: [HACKERS] Strange Windows problem, lock_timeout test request

[Hari Babu]

On Saturday, January 19, 2013 11:23 AM Amit kapila wrote:
>On Saturday, January 19, 2013 4:13 AM Boszormenyi Zoltan wrote:
>> Hi,
>>
>> Unfortunately, I won't have time to do anything with my lock_timeout
patch
>> for about 3 weeks. Does anyone have a little spare time to test it on
Windows?
>
>I shall try to do it, probably next week.
>Others are also welcome to test the patch.

I have carried out some windows testing of the lock timeout patch. 

The extra tests which are carried out on the patch are attached in the mail.


Some observations on the patch: 

1. Patch needs a rebase as it causing some rejections. 
2. regress check failed because the expected ".out" file is not updated
properly. 


Regards,
Hari babu.


lock_timeout_test.sql
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] Re: Doc patch making firm recommendation for setting the value of commit_delay

[Noah Misch]

On Mon, Jan 28, 2013 at 04:48:56AM +, Peter Geoghegan wrote:
> On 28 January 2013 03:34, Noah Misch  wrote:
> > Would you commit to the same git repository the pgbench-tools data for the
> > graphs appearing in that blog post?  I couldn't readily tell what was
> > happening below 16 clients due to the graphed data points blending together.
> 
> I'm afraid that I no longer have that data. Of course, I could fairly
> easily recreate it, but I don't think I'll have time tomorrow. Is it
> important? Are you interested in both the insert and tpc-b cases?

No need, then.


-- 
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] lazy_vacuum_heap()'s removal of HEAPTUPLE_DEAD tuples

[Simon Riggs]

On 23 January 2013 04:35, Noah Misch  wrote:

>> Also, perhaps we should
>> consider Simon's one-liner fix for backpatching this instead of the
>> original patch you posted?
>
> I have no nontrivial preference between the two approaches.

Sorry, I didn't see this. I guess you saw I applied my one liner and
backpatched it.

I'm expecting to apply Noah's larger patch to HEAD with the suggested
edits. I'll do that last thing in the CF.

-- 
 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] autovacuum not prioritising for-wraparound tables

[Andres Freund]

On 2013-01-29 00:11:12 +1100, Josh Berkus wrote:
> 
> > So I think we need to sort by age(relfrozenxid) in tables that are over
> > the anti-wraparound limit. Given your code that doesn't seem to be that
> > hard?
> 
> I might also suggest that we think about changing the defaults for
> wraparound vacuum behavior.  Partcularly, the fact that
> vacuum_freeze_min_age is 50% of autovacuum_freeze_max_age by default is
> optimal for absolutely nobody, and forces re-wraparound vacuuming of
> wraparound tables which were just recently wraparound-vacuumed.  We
> should lower vacuum_freeze_min_age to something sane, like 100.

I have to admit, I fail to see why this is a good idea. There isn't much
of an efficiency bonus in freezing early (due to hint bits) and vacuums
over vacuum_freeze_table_age are considerably more expensive as they
have to scan the whole heap instead of using the visibilitymap. And if
you don't vacuum the whole heap you can't lower relfrozenxid. So
changing freeze_min_age doesn't help at all to avoid anti-wraparound
vacuums.

Am I missing something?

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] lazy_vacuum_heap()'s removal of HEAPTUPLE_DEAD tuples

[Pavan Deolasee]

On Wed, Jan 23, 2013 at 10:05 AM, Noah Misch  wrote:

> You're the second commentator to be skittish about the patch's correctness, so
> I won't argue against a conservatism-motivated bounce of the patch.

Can you please rebase the patch against the latest head ? I see
Alvaro's and Simon's recent changes has bit-rotten the patch.

Thanks,
Pavan

-- 
Pavan Deolasee
http://www.linkedin.com/in/pavandeolasee


-- 
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: Patches for TODO item: Avoid truncating empty OCDR temp tables on COMMIT

[Heikki Linnakangas]

On 15.01.2013 21:03, Tom Lane wrote:

Robert Haas  writes:

On the third hand, the fact that a table is OCDR is recorded in
backend-local storage somewhere, and that storage (unlike the
relcache) had better be reliable.  So maybe there's some way to
finesse it that way.


Hm, keep a flag in that storage you mean?  Yeah, that could possibly
work.


Sounds reasonable.

Until someone gets around to write a patch along those lines, I'm 
inclined to apply this one liner:


*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***
*** 10124,10130  PreCommit_on_commit_actions(void)
/* Do nothing (there shouldn't be such entries, 
actually) */
break;
case ONCOMMIT_DELETE_ROWS:
!   oids_to_truncate = lappend_oid(oids_to_truncate, 
oc->relid);
break;
case ONCOMMIT_DROP:
{
--- 10124,10136 
/* Do nothing (there shouldn't be such entries, 
actually) */
break;
case ONCOMMIT_DELETE_ROWS:
!   /*
!* If this transaction hasn't accessed any 
temporary relations,
!* we can skip truncating ON COMMIT DELETE ROWS 
tables, as
!* they must still be empty.
!*/
!   if (MyXactAccessedTempRel)
!   oids_to_truncate = 
lappend_oid(oids_to_truncate, oc->relid);
break;
case ONCOMMIT_DROP:
{

We already have that MyXactAccessedTempRel global flag. Just checking 
that should cover many common cases.


- 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] autovacuum not prioritising for-wraparound tables

[Josh Berkus]

> So I think we need to sort by age(relfrozenxid) in tables that are over
> the anti-wraparound limit. Given your code that doesn't seem to be that
> hard?

I might also suggest that we think about changing the defaults for
wraparound vacuum behavior.  Partcularly, the fact that
vacuum_freeze_min_age is 50% of autovacuum_freeze_max_age by default is
optimal for absolutely nobody, and forces re-wraparound vacuuming of
wraparound tables which were just recently wraparound-vacuumed.  We
should lower vacuum_freeze_min_age to something sane, like 100.

(background:
http://www.databasesoup.com/2012/10/freezing-your-tuples-off-part-2.html)

Also, while I don't know if Alvaro's optimization is a net gain or not
(It might be), I do agree that backpatching it is not worth considering.

-- 
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] pg_catalog

[Fabrízio de Royes Mello]

On Mon, Jan 28, 2013 at 10:24 AM, Graham Little 
wrote:
>
> Hi,
>
>
>
> I have tried other sources but to no avail. Could someone please tell me
which tables in pg_catalog
> are effected by creating or dropping a trigger. If there is a work flow
diagram or source code location
> you want to point me to rather than listing them that will be fine also.

I don't know if its completely right, but when a trigger is created the
following catalog are affecteds:
- pg_trigger (new entry)
- pg_depend (new entry)
- pg_class (update relhastriggers column)


> Some plonker deleted some rows directly in the pg_triggers table …. I
need to fix it.

Maybe you simply DROP and CREATE trigger solve it...

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello

[HACKERS] pg_catalog

[Graham Little]

Hi,

I have tried other sources but to no avail. Could someone please tell me which 
tables in pg_catalog are effected by creating or dropping a trigger. If there 
is a work flow diagram or source code location you want to point me to rather 
than listing them that will be fine also. Some plonker deleted some rows 
directly in the pg_triggers table  I need to fix it.

Many Thanks

Graham

Re: [HACKERS] Support for REINDEX CONCURRENTLY

[Andres Freund]

On 2013-01-28 20:50:21 +0900, Michael Paquier wrote:
> On Mon, Jan 28, 2013 at 8:44 PM, Andres Freund  wrote:
> 
> > > Another argument that would be enough for a rejection of this patch by a
> > > committer is the problem of invalid toast indexes that cannot be removed
> > up
> > > cleanly by an operator. As long as there is not a clean solution for
> > > that...
> >
> > I think that part is relatively easy to fix, I wouldn't worry too
> > much.
> > The more complex part is how to get tuptoaster.c to update the
> > concurrently created index. That's what I worry about. Its not going
> > through the normal executor paths but manually updates the toast
> > index - which means it won't update the indisready && !indisvalid
> > index...
> >
> I included in the patch some stuff to update the reltoastidxid of the
> parent relation of the toast index. Have a look at
> index.c:index_concurrent_swap. The particular case I had in mind was if
> there is a failure of the server during the concurrent reindex of a toast
> index. 

Thats not enough unfortunately. The problem scenario is the following:

toast table: pg_toast.pg_toast_16384
toast index (via reltoastidxid): pg_toast.pg_toast_16384_index
REINDEX CONCURRENTLY PHASE #1
REINDEX CONCURRENTLY PHASE #2
toast table: pg_toast.pg_toast_16384
toast index (via reltoastidxid): pg_toast.pg_toast_16384_index, ready & valid
toast index (via pg_index): pg_toast.pg_toast_16384_index_tmp, ready & !valid

If a tuple gets toasted in this state tuptoaster.c will update
16384_index but not 16384_index_tmp. In normal tables this works because
nodeModifyTable uses ExecInsertIndexTuples which updates all ready
indexes. tuptoaster.c does something different though, it calls
index_insert exactly on the one expected index, not on the other ones.

Makes sense?

> When server restarts, the toast relation will have an invalid index
> and this cannot be dropped by an operator via SQL.

That requires about two lines of special case code in
RangeVarCallbackForDropRelation, that doesn't seem to be too bad to me.

I.e. allow the case where its IsSystemClass(classform) && relkind ==
RELKIND_INDEX && !indisvalid.

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] Support for REINDEX CONCURRENTLY

[Michael Paquier]

On Mon, Jan 28, 2013 at 8:44 PM, Andres Freund  wrote:

> > Another argument that would be enough for a rejection of this patch by a
> > committer is the problem of invalid toast indexes that cannot be removed
> up
> > cleanly by an operator. As long as there is not a clean solution for
> > that...
>
> I think that part is relatively easy to fix, I wouldn't worry too
> much.
> The more complex part is how to get tuptoaster.c to update the
> concurrently created index. That's what I worry about. Its not going
> through the normal executor paths but manually updates the toast
> index - which means it won't update the indisready && !indisvalid
> index...
>
I included in the patch some stuff to update the reltoastidxid of the
parent relation of the toast index. Have a look at
index.c:index_concurrent_swap. The particular case I had in mind was if
there is a failure of the server during the concurrent reindex of a toast
index. When server restarts, the toast relation will have an invalid index
and this cannot be dropped by an operator via SQL.
-- 
Michael Paquier
http://michael.otacoo.com

Re: [HACKERS] Support for REINDEX CONCURRENTLY

[Andres Freund]

Hi,

On 2013-01-28 20:31:48 +0900, Michael Paquier wrote:
> On Mon, Jan 28, 2013 at 7:39 PM, Andres Freund wrote:
>
> > On 2013-01-27 07:54:43 +0900, Michael Paquier wrote:
> > I think you're misunderstanding how this part works a bit. We don't
> > acquire locks on the table itself, but we get a list of all transactions
> > we would conflict with if we were to acquire a lock of a certain
> > strength on the table (GetLockConflicts(locktag, mode)). We then wait
> > for each transaction in the resulting list via the VirtualXact mechanism
> > (VirtualXactLock(*lockholder)).
> > It doesn't matter all that waiting happens in the same transaction the
> > initial index build is done in as long as we keep the session locks
> > preventing other schema modifications. Nobody can go back and see an
> > older index list after we've done the above wait once.
> >
> Don't worry I got it. I just thought that it was necessary to wait for the
> locks taken on the parent relation by other backends just *before* building
> the index. It seemed more stable.

I don't see any need for that. Its really only about making sure their
relcache entry for the indexlist - and by extension rd_indexattr - in
all other transactions that could possibly write to the table is
up2date.

As a relation_open with a lock (which is done for every write) will
always drain the invalidations thats guaranteed if we wait that way.

> So the following should be perfectly fine:
> >
> > StartTransactionCommand();
> > BuildListOfIndexes();
> > foreach(index in indexes)
> > DefineNewIndex(index);
> > CommitTransactionCommand();
> >
> > StartTransactionCommand();
> > foreach(table in tables)
> > GetLockConflicts()
> > foreach(conflict in conflicts)
> > VirtualXactLocks()
> > CommitTransactionCommand();
> >
> > foreach(index in indexes)
> > StartTransactionCommand();
> > InitialIndexBuild(index)
> > CommitTransactionCommand();
> >
> So you're point is simply to wait for all the locks currently taken on each
> table in a different transaction only once and for all, independently from
> the build and validation phases. Correct?

Exactly. That will batch the wait for the transactions together and thus
will greatly decrease the overhead of doing a concurrent reindex
(wall, not cpu-clock wise).

> > > It looks that this feature has still too many disadvantages compared to 
> > > the
> > > advantages it could bring in the current infrastructure (SnapshotNow
> > > problems, what to do with invalid toast indexes, etc.), so I would tend to
> > > agree with Tom and postpone this feature once infrastructure is more
> > > mature, one of the main things being the non-MVCC'ed catalogs.
> >
> > I think while catalog mvcc snapshots would make this easier, most
> > problems, basically all but the switching of relations, are pretty much
> > independent from that fact. All the waiting etc, will still be there.
> >
> > I can see an argument for pushing it to the next CF because its not
> > really there yet...
> >
> Even if we get this patch in a shape that you think is sufficient to make
> it reviewable by a committer within a couple of days, there are still many
> doubts from many people regarding this feature, so this is going to take
> far more time to put it in a shape that would satisfy a vast majority. So
> it is honestly wiser to work on that later.

I really haven't heard too many arguments from other after the initial
round.
Right now I "only" recall Tom and Robert doubting the usefulness, right?

I think most of the work in this patch is completely independent from
the snapshot stuff, so I really don't see much of an argument to make it
dependent on catalog snapshots.

> Another argument that would be enough for a rejection of this patch by a
> committer is the problem of invalid toast indexes that cannot be removed up
> cleanly by an operator. As long as there is not a clean solution for
> that...

I think that part is relatively easy to fix, I wouldn't worry too
much.
The more complex part is how to get tuptoaster.c to update the
concurrently created index. Thats what I worry about. Its not going
through the normal executor paths but manually updates the toast
index - which means it won't update the indisready && !indisvalid
index...

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_dump --pretty-print-views

[Jan Urbański]

On 28/01/13 12:31, Marko Tiikkaja wrote:

On 1/28/13 12:14 PM, Jeevan Chalke wrote:

I could not apply the patch with git apply, but able to apply it by patch
-p1 command.


IME that's normal for patches that went through filterdiff.  I do: git
diff |filterdiff --format=context  to re-format the patches to the
context diff preferred on the mailing list.  Maybe if I somehow told git
to produce context diff it would work..


Try this, worked for me:

http://wiki.postgresql.org/wiki/Working_with_Git#Context_diffs_with_Git

Cheers,
Jan


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for REINDEX CONCURRENTLY

[Michael Paquier]

On Mon, Jan 28, 2013 at 7:39 PM, Andres Freund wrote:

> On 2013-01-27 07:54:43 +0900, Michael Paquier wrote:
> I think you're misunderstanding how this part works a bit. We don't
> acquire locks on the table itself, but we get a list of all transactions
> we would conflict with if we were to acquire a lock of a certain
> strength on the table (GetLockConflicts(locktag, mode)). We then wait
> for each transaction in the resulting list via the VirtualXact mechanism
> (VirtualXactLock(*lockholder)).
> It doesn't matter all that waiting happens in the same transaction the
> initial index build is done in as long as we keep the session locks
> preventing other schema modifications. Nobody can go back and see an
> older index list after we've done the above wait once.
>
Don't worry I got it. I just thought that it was necessary to wait for the
locks taken on the parent relation by other backends just *before* building
the index. It seemed more stable.

So the following should be perfectly fine:
>
> StartTransactionCommand();
> BuildListOfIndexes();
> foreach(index in indexes)
> DefineNewIndex(index);
> CommitTransactionCommand();
>
> StartTransactionCommand();
> foreach(table in tables)
> GetLockConflicts()
> foreach(conflict in conflicts)
> VirtualXactLocks()
> CommitTransactionCommand();
>
> foreach(index in indexes)
> StartTransactionCommand();
> InitialIndexBuild(index)
> CommitTransactionCommand();
>
So you're point is simply to wait for all the locks currently taken on each
table in a different transaction only once and for all, independently from
the build and validation phases. Correct?


> It looks that this feature has still too many disadvantages compared to
> the
> > advantages it could bring in the current infrastructure (SnapshotNow
> > problems, what to do with invalid toast indexes, etc.), so I would tend
> to
> > agree with Tom and postpone this feature once infrastructure is more
> > mature, one of the main things being the non-MVCC'ed catalogs.
>
> I think while catalog mvcc snapshots would make this easier, most
> problems, basically all but the switching of relations, are pretty much
> independent from that fact. All the waiting etc, will still be there.
>
> I can see an argument for pushing it to the next CF because its not
> really there yet...
>
Even if we get this patch in a shape that you think is sufficient to make
it reviewable by a committer within a couple of days, there are still many
doubts from many people regarding this feature, so this is going to take
far more time to put it in a shape that would satisfy a vast majority. So
it is honestly wiser to work on that later.

Another argument that would be enough for a rejection of this patch by a
committer is the problem of invalid toast indexes that cannot be removed up
cleanly by an operator. As long as there is not a clean solution for that...
-- 
Michael Paquier
http://michael.otacoo.com

Re: [HACKERS] Re: logical changeset generation v4 - Heikki's thoughts about the patch state

[Andres Freund]

On 2013-01-28 11:59:52 +0200, Heikki Linnakangas wrote:
> On 24.01.2013 00:30, Andres Freund wrote:
> >Also, while the apply side surely isn't benchmarkable without any being
> >submitted, the changeset generation can very well be benchmarked.
> >
> >A very, very adhoc benchmark:
> >  -c max_wal_senders=10
> >  -c max_logical_slots=10 --disabled for anything but logical
> >  -c wal_level=logical --hot_standby for anything but logical
> >  -c checkpoint_segments=100
> >  -c log_checkpoints=on
> >  -c shared_buffers=512MB
> >  -c autovacuum=on
> >  -c log_min_messages=notice
> >  -c log_line_prefix='[%p %t] '
> >  -c wal_keep_segments=100
> >  -c fsync=off
> >  -c synchronous_commit=off
> >
> >pgbench -p 5440 -h /tmp -n -M prepared -c 16 -j 16 -T 30
> >
> >pgbench upstream:
> >tps: 22275.941409
> >space overhead: 0%
> >pgbench logical-submitted
> >tps: 16274.603046
> >space overhead: 2.1%
> >pgbench logical-HEAD (will submit updated version tomorrow or so):
> >tps: 20853.341551
> >space overhead: 2.3%
> >pgbench single plpgsql trigger (INSERT INTO log(data) VALUES(NEW::text))
> >tps: 14101.349535
> >space overhead: 369%
> >
> >Note that in the single trigger case nobody consumed the queue while the
> >logical version streamed the changes out and stored them to disk.
> 
> That makes the space overhead comparison completely worthless, no? I would
> expect the trigger-based approach to generate roughly 100% more WAL, not
> close to 400%. As long as the queue is drained constantly, there should be
> no big difference in the disk space used, except for the WAL.

Imo its a valid comparison as all such queues can only be drained in a
rather imperfect manner. I think these days all solutions use multiple
(two) queue tables and switch between those and truncate the non-active
one as vacuuming them works far too unreliable.
And those tables have to be plain logged once, so they matter in
checkpoints et al.

> >Adding a default NOW() or similar to the tables immediately makes
> >logical decoding faster by a factor of about 3 in comparison to the
> >above trivial trigger.
> 
> Hmm, is that because of the conversion to text? I believe slony also
> converts all the values to text in the trigger, because that's simple and
> flexible, but if we're trying to compare the performance of logical
> changeset generation vs. trigger-based replication in general, we should
> choose the most efficient trigger-based scheme to compare with. That means,
> don't convert to text. And write the trigger in C.

Imo its basically impossible for the current queue-based solutions not
to convert to text because they otherwise would need to queue all the
conversion information as well. And the the test_decoding plugin also
converts everything to text, so thats a fair comparison from that
POV. In fact the test_decoding plugin does noticeably more as it also
outputs table, column and type name.

I aggree on the C argument. I really doubt its going to make that much
of a difference but we should try it.
In my experience a plpgsql trigger that just does a straight conversion
via cast is still noticeably faster than any of the "real" replication
triggers out there though, so I wouldn't expect much there.


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_dump --pretty-print-views

[Marko Tiikkaja]

On 1/28/13 12:14 PM, Jeevan Chalke wrote:

I could not apply the patch with git apply, but able to apply it by patch
-p1 command.


IME that's normal for patches that went through filterdiff.  I do: git 
diff |filterdiff --format=context  to re-format the patches to the 
context diff preferred on the mailing list.  Maybe if I somehow told git 
to produce context diff it would work..



However, will you please justify the changes done in "xml.out" ? I guess
they are not needed.
You might need to configure your sources with libxml.


If you look very carefully, the pretty-printing version adds one space 
at the very beginning of the output. :-)



Also, I am not sure about putting "WRAP_COLUMN_DEFAULT" by default. I will
keep that in code committors plate. Rest of the code changes looks good to
me.


Thanks for reviewing!


Regards,
Marko Tiikkaja


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical changeset generation v4

[Andres Freund]

On 2013-01-27 12:28:21 -0500, Steve Singer wrote:
> On 13-01-22 11:30 AM, Andres Freund wrote:
> >Hi,
> >
> >I pushed a new rebased version (the xlogreader commit made it annoying
> >to merge).
> >
> >The main improvements are
> >* way much coherent code internally for intializing logical rep
> >* explicit control over slots
> >* options for logical replication
> 
> 
> Exactly what is the syntax for using that.  My reading your changes to
> repl_gram.y make me think that any of the following should work (but they
> don't).
> 
> START_LOGICAL_REPLICATION 'slon1' 0/0 ('opt1')
>  ERROR:  syntax error: unexpected character "("
> 
> "START_LOGICAL_REPLICATION 'slon1' 0/0 ('opt1' 'val1')
>  ERROR:  syntax error: unexpected character "("
> 
> START_LOGICAL_REPLICATION 'slon1' 0/0 ('opt1','opt2')
> ERROR:  syntax error: unexpected character "("

The syntax is right, the grammar (or rather scanner) support is a bit
botched, will push a new version soon.
 
> I'm also attaching a patch to pg_receivellog that allows you to specify
> these options on the command line.  I'm not saying I think that it is
> appropriate to be adding more bells and whistles to the utilities  two weeks
> into the CF but I found this useful for testing so I'm sharing it.

The CF is also there to find UI warts and such, so something like this
seems perfectly fine. Even moreso as it doesn't look this will get into
9.3 anyway.

I wanted to add such an option, but I was too lazy^Wbusy to think about
the sematics. Your current syntax doesn't really allow arguments to be
specified in a nice way.
I was thinking of -o name=value and allowing multiple specifications of
-o to build the option string.

Any arguments against that?

>   /* Initiate the replication stream at specified location */
> - snprintf(query, sizeof(query), "START_LOGICAL_REPLICATION '%s' %X/%X",
> -  slot, (uint32) (startpos >> 32), (uint32) startpos);
> + snprintf(query, sizeof(query), "START_LOGICAL_REPLICATION '%s' %X/%X 
> (%s)",
> +  slot, (uint32) (startpos >> 32), (uint32) 
> startpos,plugin_opts);

ISTM that (%s) shouldn't be specified when there are no options, but as
the options need to be pre-escaped anyway, that looks like a non-problem
in a bit more complete implementation.

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] logical changeset generation v4 - Heikki's thoughts about the patch state

[Andres Freund]

Hi,

On 2013-01-27 23:07:51 -0500, Steve Singer wrote:
> A few more comments;
> 
> In decode.c DecodeDelete
> 
> +   if (r->xl_len <= (SizeOfHeapDelete + SizeOfHeapHeader))
> +   {
> +   elog(DEBUG2, "huh, no primary key for a delete on wal_level =
> logical?");
> +   return;
> +   }
> +
 
> I think we should be passing delete's with candidate key data logged to the
> plugin.  If the table isn't a replicated table then ignoring the delete is
> fine.  If the table is a replicated table but someone has deleted the unique
> index from the table then the plugin will receive INSERT changes on the
> table but not DELETE changes. If this happens the plugin would have any way
> of knowing that it is missing delete changes.  If my plugin gets passed a
> DELETE change record but with no key data then my plugin could do any of

I basically didn't do that because I thought people would forget to
check whether oldtuple is empty I have no problem with addind support
for that though.

> 1.  Start screaming for help (ie log errors)

Yes.

> 2.  Drop the table from replication

No, you can't write from an output plugin, and I don't immediately see
support for that comming. There's no fundamental blockers, just makes
things more complicated.

> 3.  Pass the delete (with no key values) onto the replication client and let
> it deal with it (see 1 and 2)

Hm.


While I agree that nicer behaviour would be good I think the real
enforcement should happen on a higher level, e.g. with event triggers or
somesuch. It seems way too late to do anything about it when we're
already decoding. The transaction will already have committed...

> Also, 'huh' isn't one of our standard log message phrases :)

You're right there ;). I bascially wanted to remove the log message
almost instantly but it was occasionally useful so I kept it arround...

> How do you plan on dealing with sequences?
> I don't see my plugin being called on sequence changes and I don't see
> XLOG_SEQ_LOG listed in DecodeRecordIntoReorderBuffer.  Is there a reason why
> this can't be easily added?

I basically was hoping for Simon's sequence-am to get in before doing
anything real here. That didn't really happen yet.
I am not sure whether there's a real usecase in decoding normal
XLOG_SEQ_LOG records, their content isn't all that easy to interpet
unless youre rather familiar with pg's innards.

So, adding support wouldn't hard from a technical pov but it seems the
semantics are a bit hard to nail down.

> Also what do we want to do about TRUNCATE support.  I could always leave a
> TRUNCATE trigger in place that logged the truncate to a sl_truncates and
> have my replication daemon respond to the insert on a   sl_truncates table
> by actually truncating the data on the replica.

I have planned to add some generic "table_rewrite" handling, but I have
to admit I haven't thought too much about it yet. Currently if somebody
rewrites a table, e.g. with an ALTER ... ADD COLUMN .. DEFAULT .. or
ALTER COLUMN ... USING ..., you will see INSERTs into a temporary
table. That basically seems to be a good thing, but the user needs to be
told about that ;)

> I've spent some time this weekend updating my prototype plugin that
> generates slony 2.2 style COPY output.  I have attached my progress here
> (also https://github.com/ssinger/slony1-engine/tree/logical_repl).  I have
> not gotten as far as modifying slon to act as a logical log receiver, or
> made a version of the slony apply trigger that would process these
> changes.

I only gave it a quick look and have a couple of questions and
remarks. The way you used the options it looks like youre thinking of
specifying all the tables as options? I would have thought those would
get stored & queried locally and only something like the 'replication
set' name or such would be set as an option.

Iterating over a list with
for(i = 0; i < options->length; i= i + 2 )
{
DefElem * def_schema = (DefElem*) list_nth(options,i);
is not a good idea btw, thats quadratic in complexity ;)

In the REORDER_BUFFER_CHANGE_UPDATE I suggest using
relation->rd_primary, just as in the DELETE case, that should always
give you a consistent candidate key in an efficient manner.

> I haven't looked into the details of what is involved in setting up a
> subscription with the snapshot exporting.

That hopefully shouldn't be too hard... At least thats the idea :P

> I couldn't get the options on the START REPLICATION command to parse so I
> just hard coded some list building code in the init method. I do plan on
> pasing the list of tables to replicate from the replica to the plugin
> (because this list comes from the replica).   Passing what could be a few
> thousand table names as a list of arguments is a bit ugly and I admit my
> list processing code is rough.  Does this make us want to reconsider the
> format of the option_list ?

Yea, something's screwed up there, sorry. Will push a fix later today.

> I guess should provide

Re: [HACKERS] pg_dump --pretty-print-views

[Jeevan Chalke]

Hi Marko,

I could not apply the patch with git apply, but able to apply it by patch
-p1 command.

However, will you please justify the changes done in "xml.out" ? I guess
they are not needed.
You might need to configure your sources with libxml.

Also, I am not sure about putting "WRAP_COLUMN_DEFAULT" by default. I will
keep that in code committors plate. Rest of the code changes looks good to
me.

Thanks

On Sun, Jan 27, 2013 at 6:23 PM, Marko Tiikkaja  wrote:

> On Fri, 18 Jan 2013 07:46:21 +0100, Jeevan Chalke <
> jeevan.chalke@enterprisedb.**com > wrote:
>
>> Nice idea.
>>
>> Marko, just hack ruleutils some thing like this:
>>
>
> Here's a patch attempting to do just that.
>
> The actual code changes were trivial, the patch is mostly just regression
> tests.
>
> As for the docs, I wasn't entirely happy with what they say about
> pg_get_viewdef(), but it didn't look like they needed to be changed by this
> either.
>
>
> Regards,
> Marko Tiikkaja




-- 
Jeevan B Chalke
Senior Software Engineer, R&D
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Phone: +91 20 30589500

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

This e-mail message (and any attachment) is intended for the use of the
individual or entity to whom it is addressed. This message contains
information from EnterpriseDB Corporation that may be privileged,
confidential, or exempt from disclosure under applicable law. If you are
not the intended recipient or authorized to receive this for the intended
recipient, any use, dissemination, distribution, retention, archiving, or
copying of this communication is strictly prohibited. If you have received
this e-mail in error, please notify the sender immediately by reply e-mail
and delete this message.

[HACKERS] Number of buckets in a hash join

[Heikki Linnakangas]

While testing Alexander's gistchoose patch, "perf report" showed that 
the test case spent a surprisingly large amount of CPU time in 
ExecScanHashBucket. That function just scans a hash bucket for matches, 
and it should be very quick as long as there are not many collisions.


It turns out that the planner estimated the number of rows in the hash 
to be much smaller than it actually contained, and the hash table was 
initialized with too few buckets as a result. The target is that each 
bucket contains 10 tuples (NTUP_PER_BUCKET), but in this case, the 
average was about 100.


The first question is, why do we aim at 10 tuples per bucket? My gut 
feeling is that that's way too high. I would expect the target to be 1 
tuple per bucket, or maybe a little more, like 2-3. Each bucket consumes 
one pointer's worth of RAM, which is not much. There's also some 
overhead from empty buckets when scanning the hash table, but as long as 
all the buckets have at least one entry, there's no gain from having 
more than one entry per bucket.


However, lowering NTUP_PER_BUCKET would not have helped in this case, 
because we also have a minimum of 1024 buckets. The estimate was so bad 
that even after setting NTUP_PER_BUCKET to 1, it was still pegged at 
that minimum of 1024 buckets.


Ideally, the planner would always make a good guess the number of rows, 
but for the situations that it doesn't, it would be good if the hash 
table was enlarged if it becomes too full. It's a well-known technique 
to double the size of a hash table once the load factor reaches a 
certain threshold, and rehash the existing entries. Another idea is to 
just collect all the entries in e.g a linked list when tuples are 
inserted to the hash table, and create the buckets lazily, after all the 
tuples have been inserted.


Here's an extreme example of this phenomenon. According to perf, about 
95% of the CPU time is spent in ExecScanHashBucket. That would be 
eliminated by sizing the hash table correctly:


create table numbers (id int4);
insert into numbers select g from generate_series(1, 1000) g;

explain analyze select * from numbers a, generate_series(1, 10) b 
where b = a.id;
  QUERY 
PLAN


-
--
 Hash Join  (cost=22.50..2035430.50 rows=53097600 width=8) (actual 
time=32.307..2

9141.348 rows=10 loops=1)
   Hash Cond: (a.id = b.b)
   ->  Seq Scan on numbers a  (cost=0.00..150443.20 rows=10619520 
width=4) (actua

l time=0.017..716.503 rows=1000 loops=1)
   ->  Hash  (cost=10.00..10.00 rows=1000 width=4) (actual 
time=32.268..32.268 ro

ws=10 loops=1)
 Buckets: 1024  Batches: 1  Memory Usage: 3516kB
 ->  Function Scan on generate_series b  (cost=0.00..10.00 
rows=1000 widt

h=4) (actual time=17.966..22.519 rows=10 loops=1)
 Total runtime: 29146.011 ms
(7 rows)


- 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] logical changeset generation v4 - Heikki's thoughts about the patch state

[Andres Freund]

On 2013-01-26 16:20:33 -0500, Steve Singer wrote:
> On 13-01-24 11:15 AM, Steve Singer wrote:
> >On 13-01-24 06:40 AM, Andres Freund wrote:
> >>
> >>Fair enough. I am also working on a user of this infrastructure but that
> >>doesn't help you very much. Steve Singer seemed to make some stabs at
> >>writing an output plugin as well. Steve, how far did you get there?
> >
> >I was able to get something that generated output for INSERT statements in
> >a format similar to what a modified slony apply trigger would want.  This
> >was with the list of tables to replicate hard-coded in the plugin.  This
> >was with the patchset from the last commitfest.I had gotten a bit hung up
> >on the UPDATE and DELETE support because slony allows you to use an
> >arbitrary user specified unique index as your key.  It looks like better
> >support for tables with a unique non-primary key is in the most recent
> >patch set.  I am hoping to have time this weekend to update my plugin to
> >use parameters passed in on the init and other updates in the most recent
> >version.  If I make some progress I will post a link to my progress at the
> >end of the weekend.  My big issue is that I have limited time to spend on
> >this.
> >
>
> This isn't a complete review just a few questions I've hit so far that I
> thought I'd ask to see if I'm not seeing something related to updates.

> + extern void relationFindPrimaryKey(Relation pkrel, Oid *indexOid,
> +int16 *nratts, int16 *attnums, Oid
> *atttypids,
> +Oid *opclasses);
> +
>
> I don't see this defined anywhere could it be left over from a previous
> version of the patch?

Yes, its dead and now gone.

> In decode.c
> DecodeUpdate:
> +
> +   /*
> +* FIXME: need to get/save the old tuple as well if we want primary key
> +* changes to work.
> +*/
> +   change->newtuple = ReorderBufferGetTupleBuf(reorder);
>
> I also don't see any code in heap_update to find + save the old primary key
> values like you added to heap_delete.   You didn't list "Add ability to
> change the primary key on an UPDATE" in the TODO so I'm wondering if I'm
> missing something.  Is there another way I can bet the primary key values
> for the old_tuple?

Nope, there isn't any right now. I have considered as something not all
that interesting for real-world usecases based on my experience, but
adding support shouldn't be that hard anymore, so I can just bite the
bullet...

> I think the name of the test contrib module was changed but you didn't
> update the make file. This fixes it

Yea, I had forgotten to add that hunk when committing. Fixed.

Thanks,

Andres

--
 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] Support for REINDEX CONCURRENTLY

[Andres Freund]

On 2013-01-27 07:54:43 +0900, Michael Paquier wrote:
> On Sun, Jan 27, 2013 at 1:52 AM, Andres Freund wrote:
> > On 2013-01-25 13:48:50 +0900, Michael Paquier wrote:
> > > > As far as I understand that code its purpose is to enforce that all
> > > > potential users have an up2date definition available. For that we
> > > > acquire a lock on all virtualxids of users using that table thus waiting
> > > > for them to finish.
> > > > Consider the scenario where you have a workload where most transactions
> > > > are fairly long (say 10min) and use the same tables (a,b)/indexes(a_1,
> > > > a_2, b_1, b_2). With the current strategy you will do:
> > > >
> > > > WaitForVirtualLocks(a_1) -- wait up to 10min
> > > > index_build(a_1)
> > > > WaitForVirtualLocks(a_2) -- wait up to 10min
> > > > index_build(a_2)
> > > >
> > > ...
> > > >
> > > > So instead of waiting up 10 minutes for that phase you have to wait up
> > > > to 40.
> > > >
> > > This is necessary if you want to process each index entry in a different
> > > transaction as WaitForVirtualLocks needs to wait for the locks held on the
> > > parent table. If you want to fo this wait once per transaction, the
> > > solution would be to group the index builds in the same transaction for 
> > > all
> > > the indexes of the relation. One index per transaction looks more solid in
> > > this case if there is a failure during a process only one index will be
> > > incorrectly built.
> >
> > I cannot really follow you here.
> >
> OK let's be more explicit...

> > The reason why we need to wait here is
> > *only* to make sure that nobody still has the old list of indexes
> > around (which probably could even be relaxed for reindex concurrently,
> > but thats a separate optimization).
> >
> In order to do that, you need to wait for the *parent relations* and not
> the index themselves, no?
> Based on 2 facts:
> - each index build is done in a single transaction
> - a wait needs to be done on the parent relation before each transaction
> You need to wait for the parent relation multiple times depending on the
> number of indexes in it. You could optimize that by building all the
> indexes of the *same parent relation* in a single transaction.

I think youre misunderstanding how this part works a bit. We don't
acquire locks on the table itself, but we get a list of all transactions
we would conflict with if we were to acquire a lock of a certain
strength on the table (GetLockConflicts(locktag, mode)). We then wait
for each transaction in the resulting list via the VirtualXact mechanism
(VirtualXactLock(*lockholder)).
It doesn't matter all that waiting happens in the same transaction the
initial index build is done in as long as we keep the session locks
preventing other schema modifications. Nobody can go back and see an
older index list after we've done the above wait once.

So the following should be perfectly fine:

StartTransactionCommand();
BuildListOfIndexes();
foreach(index in indexes)
DefineNewIndex(index);
CommitTransactionCommand();

StartTransactionCommand();
foreach(table in tables)
GetLockConflicts()
foreach(conflict in conflicts)
VirtualXactLocks()
CommitTransactionCommand();

foreach(index in indexes)
StartTransactionCommand();
InitialIndexBuild(index)
CommitTransactionCommand();
...


> It looks that this feature has still too many disadvantages compared to the
> advantages it could bring in the current infrastructure (SnapshotNow
> problems, what to do with invalid toast indexes, etc.), so I would tend to
> agree with Tom and postpone this feature once infrastructure is more
> mature, one of the main things being the non-MVCC'ed catalogs.

I think while catalog mvcc snapshots would make this easier, most
problems, basically all but the switching of relations, are pretty much
independent from that fact. All the waiting etc, will still be there.

I can see an argument for pushing it to the next CF because its not
really there yet...

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: logical changeset generation v4 - Heikki's thoughts about the patch state

[Heikki Linnakangas]

On 24.01.2013 00:30, Andres Freund wrote:

Also, while the apply side surely isn't benchmarkable without any being
submitted, the changeset generation can very well be benchmarked.

A very, very adhoc benchmark:
  -c max_wal_senders=10
  -c max_logical_slots=10 --disabled for anything but logical
  -c wal_level=logical --hot_standby for anything but logical
  -c checkpoint_segments=100
  -c log_checkpoints=on
  -c shared_buffers=512MB
  -c autovacuum=on
  -c log_min_messages=notice
  -c log_line_prefix='[%p %t] '
  -c wal_keep_segments=100
  -c fsync=off
  -c synchronous_commit=off

pgbench -p 5440 -h /tmp -n -M prepared -c 16 -j 16 -T 30

pgbench upstream:
tps: 22275.941409
space overhead: 0%
pgbench logical-submitted
tps: 16274.603046
space overhead: 2.1%
pgbench logical-HEAD (will submit updated version tomorrow or so):
tps: 20853.341551
space overhead: 2.3%
pgbench single plpgsql trigger (INSERT INTO log(data) VALUES(NEW::text))
tps: 14101.349535
space overhead: 369%

Note that in the single trigger case nobody consumed the queue while the
logical version streamed the changes out and stored them to disk.


That makes the space overhead comparison completely worthless, no? I 
would expect the trigger-based approach to generate roughly 100% more 
WAL, not close to 400%. As long as the queue is drained constantly, 
there should be no big difference in the disk space used, except for the 
WAL.



Adding a default NOW() or similar to the tables immediately makes
logical decoding faster by a factor of about 3 in comparison to the
above trivial trigger.


Hmm, is that because of the conversion to text? I believe slony also 
converts all the values to text in the trigger, because that's simple 
and flexible, but if we're trying to compare the performance of logical 
changeset generation vs. trigger-based replication in general, we should 
choose the most efficient trigger-based scheme to compare with. That 
means, don't convert to text. And write the trigger in C.


- 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: Review of "pg_basebackup and pg_receivexlog to use non-blocking socket communication", was: Re: [HACKERS] Re: [BUGS] BUG #7534: walreceiver takes long time to detect n/w breakdown

[Magnus Hagander]

On Tue, Jan 22, 2013 at 7:31 AM, Amit Kapila  wrote:
> On Monday, January 21, 2013 6:22 PM Magnus Hagander
>> On Fri, Jan 18, 2013 at 7:50 AM, Amit Kapila 
>> wrote:
>> > On Wednesday, January 16, 2013 4:02 PM Heikki Linnakangas wrote:
>> >> On 07.01.2013 16:23, Boszormenyi Zoltan wrote:
>> >> > Since my other patch against pg_basebackup is now committed,
>> >> > this patch doesn't apply cleanly, patch rejects 2 hunks.
>> >> > The fixed up patch is attached.
>> >>
>> >> Now that I look at this a high-level perspective, why are we only
>> >> worried about timeouts in the Copy-mode and when connecting? The
>> >> initial
>> >> checkpoint could take a long time too, and if the server turns into
>> a
>> >> black hole while the checkpoint is running, pg_basebackup will still
>> >> hang. Then again, a short timeout on that phase would be a bad idea,
>> >> because the checkpoint can indeed take a long time.
>> >
>> > True, but IMO, if somebody want to take basebackup, he should do that
>> when
>> > the server is not loaded.
>>
>> A lot of installations don't have such an optino, because there is no
>> time whe nthe server is not loaded.
>
> Good to know about it.
> I have always heard that customer will run background maintenance activities
> (Reindex, Vacuum Full, etc) when the server is less loaded.
> For example
> a. Billing applications in telecom, at night times they can be relatively
> less loaded.

That assumes there is a nighttime.. If you're operating in enough
timezones, that won't happen.


> b. Any databases used for Sensex transactions, they will be relatively free
> once the market is closed.
> c. Banking solutions, because transactions are done mostly in day times.

True. But those are definitely very very narrow usecases ;)

Don't get me wrong. There are a *lot* of people who have nighttimes to
do maintenance in. They are the lucky ones :) But we can't assume this
scenario.


> There will be many cases where Database server will be loaded all the times,
> if you can give some example, it will be a good learning for me.

Most internet based businesses that do business in multiple countries.
Or really, any business that has customers in multiple timezones
across the world. And even more to the point, any business who's
*customers* have customers in multiple timezones across the world,
provided they are services-based.


>> >> In streaming replication, the keep-alive messages carry additional
>> >> information, the timestamps and WAL locations, so a keepalive makes
>> >> sense at that level. But otherwise, aren't we just trying to
>> >> reimplement
>> >> TCP keepalives? TCP keepalives are not perfect, but if we want to
>> have
>> >> an application level timeout, it should be implemented in the FE/BE
>> >> protocol.
>> >>
>> >> I don't think we need to do anything specific to pg_basebackup. The
>> >> user
>> >> can simply specify TCP keepalive settings in the connection string,
>> >> like
>> >> with any libpq program.
>> >
>> > I think currently user has no way to specify TCP keepalive settings
>> from
>> > pg_basebackup, please let me know if there is any such existing way?
>>
>> You can set it through environment variables. As was discussed
>> elsewhere, it would be good to have the ability to do it natively to
>> pg_basebackup as well.
>
> Sure, already modifying the existing patch to support connection string in
> pg_basebackup and pg_receivexlog.

Good.


>> > I think specifying TCP settings is very cumbersome for most users,
>> that's
>> > the reason most standard interfaces (ODBC/JDBC) have such application
>> level
>> > timeout mechanism.
>> >
>> > By implementing in FE/BE protocol (do you mean to say that make such
>> > non-blocking behavior inside Libpq or something else), it might be
>> generic
>> > and can be used for others as well but it might need few interface
>> changes.
>>
>> If it's specifying them that is cumbersome, then that's the part we
>> should fix, rather than modifying the protocol, no?
>
> That can be done as part of point 2 of initial proposal
> (2. Support recv_timeout separately to provide a way to users who are not
> comfortable tcp keepalives).

Looking at the bigger picture, we should in that case support those on
*all* our frontend applications, and not just pg_basebackup. To me, it
makes more sense to just say "use the connection string method to
connect when you need to set these parameters". There are always going
to be some parameters that require that.


> To achieve this there can be 2 ways.
> 1. Change in FE/BE protocol - I am not sure exactly how this can be done,
> but as per Heikki this is better way of implementing it.
> 2. Make the socket as non-blocking in pg_basebackup.
>
> Advantage of Approach-1 is that if we do in such a fashion that in lower
> layers (libpq) it is addressed then all other apps (pg_basebackup, etc) can
> use it, no need to handle separately in each application.
>
> So now as changes in Approach-1 seems to be invasive, we decided to d

Re: [HACKERS] "pg_ctl promote" exit status

[Heikki Linnakangas]

On 26.01.2013 23:44, Aaron W. Swenson wrote:

On Fri, Jan 25, 2013 at 01:54:06PM -0500, Peter Eisentraut wrote:

On 1/12/13 3:30 PM, Aaron W. Swenson wrote:

The Linux Standard Base Core Specification 3.1 says this should return
'3'. [1]

[1] 
http://refspecs.freestandards.org/LSB_3.1.1/LSB-Core-generic/LSB-Core-generic/iniscrptact.html


The LSB spec doesn't say anything about a "promote" action.

And for the stop and reload actions that you tried to change, 3 is
"unimplemented".

There is an ongoing discussion about the exit status of the stop action
under, so
let's keep this item about the "promote" action.


You are right. Had I read a little further down, it seems that the
exit status should actually be 7.


Not sure if that LSB section is relevant anyway. It specifies the exit 
codes for init scripts, but pg_ctl is not an init script.


- 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] unlogged tables vs. GIST

[Heikki Linnakangas]

On 23.01.2013 17:30, Robert Haas wrote:

On Wed, Jan 23, 2013 at 4:04 AM, Jeevan Chalke
  wrote:

I guess my earlier patch, which was directly incrementing
ControlFile->unloggedLSN counter was the concern as it will take
ControlFileLock several times.

In this version of patch I did what Robert has suggested. At start of the
postmaster, copying unloggedLSn value to XLogCtl, a shared memory struct.
And
in all access to unloggedLSN, using this shared variable using a SpinLock.
And since we want to keep this counter persistent across clean shutdown,
storing it in ControlFile before updating it.

With this approach, we are updating ControlFile only when we shutdown the
server, rest of the time we are having a shared memory counter. That means
we
are not touching pg_control every other millisecond or so. Also since we are
not caring about crashes, XLogging this counter like OID counter is not
required as such.


On a quick read-through this looks reasonable to me, but others may
have different opinions, and I haven't reviewed in detail.

> ...

[a couple of good points]


In addition to those things Robert pointed out:


/*
+ * Temporary GiST indexes are not WAL-logged, but we need LSNs to detect
+ * concurrent page splits anyway. GetXLogRecPtrForUnloggedRel() provides a fake
+ * sequence of LSNs for that purpose. Each call generates an LSN that is
+ * greater than any previous value returned by this function in the same
+ * session using static counter
+ * Similarily unlogged GiST indexes are also not WAL-logged. But we need a
+ * persistent counter across clean shutdown. Use counter from ControlFile which
+ * is copied in XLogCtl.unloggedLSN to accomplish that
+ * If relation is UNLOGGED, return persistent counter from XLogCtl else return
+ * session wide temporary counter
+ */
+XLogRecPtr
+GetXLogRecPtrForUnloggedRel(Relation rel)


From a modularity point of view, it's not good that you xlog.c needs to 
know about Relation struct. Perhaps move the logic to check the relation 
is unlogged or not to a function in gistutil.c, and only have a small 
GetUnloggedLSN() function in xlog.c


I'd suggest adding a separate spinlock to protect unloggedLSN. I'm not 
sure if there's much contention on XLogCtl->info_lck today, but 
nevertheless it'd be better to keep this separate, also from a 
modularity point of view.



@@ -7034,6 +7078,8 @@ CreateCheckPoint(int flags)
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
ControlFile->state = DB_SHUTDOWNING;
ControlFile->time = (pg_time_t) time(NULL);
+   /* Store unloggedLSN value as we want it persistent across 
shutdown */
+   ControlFile->unloggedLSN = XLogCtl->unloggedLSN;
UpdateControlFile();
LWLockRelease(ControlFileLock);
}


This needs to acquire the spinlock to read unloggedLSN.

Do we need to do anything to unloggedLSN in pg_resetxlog?

- 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] Re: proposal: a width specification for s specifier (format function), fix behave when positional and ordered placeholders are used

[Dean Rasheed]

On 26 January 2013 10:58, Pavel Stehule  wrote:
> updated patches due changes for better variadic "any" function.
>
> apply fix_mixing_positinal_ordered_placeholders_warnings_20130126.patch first
>

Hi,

No one is listed as a reviewer for this patch so I thought I would
take a look at it, since it looks like a useful enhancement to
format().

Starting with the first patch - it issues a new WARNING if the format
string contains a mixture of format specifiers with and without
parameter indexes (e.g., 'Hello %s, %1$s').

Having thought about it a bit, I really don't like this for a number of reasons:

* I actually quite like the current behaviour. Admittedly putting
ordered specifiers (like '%s') after positional ones (like '%3$s') is
probably not so useful, and potentially open to different
interpretations. But putting positional specifiers at the end is
completely unambiguous and can save a lot of typing (e.g.,
'%s,%s,%s,%s,%,s,%s,%s,%1$s').

* On backwards compatibility grounds. The fact that the only example
of format() in the manual is precisely a case of mixed positional and
ordered parameters makes it quite likely that people will have used
this feature already.

* Part of the justification for adding the warning is for
compatibility with glibc/SUS printf(). But if we are aiming for that,
then we should also produce a warning if positional parameters are
used and not all parameters are consumed. That would be a pain to
implement and I don't think it would be particularly helpful in
practice. Here is what the SUS says:

"""
The format can contain either numbered argument specifications (that
is, %n$ and *m$), or unnumbered argument specifications (that is, %
and *), but normally not both. The only exception to this is that %%
can be mixed with the %n$ form. The results of mixing numbered and
unnumbered argument specifications in a format string are undefined.
When numbered argument specifications are used, specifying the Nth
argument requires that all the leading arguments, from the first to
the (N-1)th, are specified in the format string.
"""

I think that if we are going to do anything, we should explicitly
document our current behaviour as a PostgreSQL extension to the SUS
printf(), describing how we handle mixed parameters, rather than
adding this warning.

The current PostgreSQL code isn't inconsistent with the SUS, except in
the error case, and so can reasonably be regarded as an extension.

Regards,
Dean


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers