Re: [HACKERS] UNION ALL on partitioned tables won't use indices.

[Noah Misch]

On Thu, Oct 24, 2013 at 07:39:53PM +0900, Kyotaro HORIGUCHI wrote:
> 1. Observed symptom
> 
> As you know, UNION ALL accompanied with ORDER BY uses indexes if
> available.

> So do simple queries on partitioned (inheritance) tables.

> Nevertheless, UNION ALL on partitioned tables doesn't. This is
> quite unfavourable behavior especially having LIMIT.
> 
> >uniontest=# EXPLAIN ANALYZE SELECT * FROM p1
> >UNION ALL SELECT * FROM p2 ORDER BY a LIMIT 10;
> >  QUERY PLAN   
> >
> > Limit   (actual time=182.732..182.735 rows=10 loops=1)
> >   ->  Sort  (actual time=182.729..182.730 rows=10 loops=1)
> > Sort Key: p1.a
> > Sort Method: top-N heapsort  Memory: 25kB
> > ->  Append  (actual time=0.029..108.109 rows=40 loops=1)
> >   ->  Seq Scan on p1  (actual time=0.001..0.001 rows=0 loops=1)
> >   ->  Seq Scan on c11  (actual time=0.027..19.074 rows=10 loops=1)
> >   ->  Seq Scan on c12  (actual time=0.014..16.653 rows=10 loops=1)
> >   ->  Seq Scan on p2  (actual time=0.000..0.000 rows=0 loops=1)
> >   ->  Seq Scan on c21  (actual time=0.012..16.677 rows=10 loops=1)
> >   ->  Seq Scan on c22  (actual time=0.012..16.794 rows=10 loops=1)
> > Total runtime: 182.857 ms

That is indeed surprising.

> 2. The cause
> 
> In grouping_planner, flattern_simple_union_all creates
> appendrelinfos for each subqueries then expand_inherited_tables
> furthur expands the parent tables in each subqueries. Finally,
> this sequence leaves following structure. Where rte[2] and [3]
> are subqueries abandoned on the way pulling up rte[4] and [5].
> 
> rte[1] Subquery "SELECT*1", inh = 1
>+- appinfo[0] - rte[4] Relation "p1", inh = 1
>|   +- appinfo[2] - rte[6]  Relation "p1", inh = 0
>|   +- appinfo[3] - rte[7]  Relation "c11", inh = 0
>|   +- appinfo[4] - rte[8]  Relation "c12", inh = 0
>+- appinfo[1] - rte[5] Relation "p2", inh = 1
>+- appinfo[5] - rte[9]  Relation "p1", inh = 0
>+- appinfo[6] - rte[10] Relation "c11", inh = 0
>+- appinfo[7] - rte[11] Relation "c12", inh = 0
> 
> On the other hand, ec member finally has exprs only for varno =
> 1, 4 and 5, in other words, it lacks the members for the most
> descendant RTEs.  This is because add_child_rel_equivalences()
> always inhibits add_eq_member from registering the child_rel's
> relid on EC. Conequently these grandchild relations does not find
> index pathkeys for themselves.

I'm unclear on the key ideas behind distinguishing em_is_child members from
ordinary EC members.  src/backend/optimizer/README says "These members are
*not* full-fledged members of the EquivalenceClass and do not affect the
class's overall properties at all."  Is that an optimization to avoid futile
planner work, or is it necessary for correctness?  If the latter, what sort of
query might give wrong answers if an EquivalenceMember were incorrectly added
as em_is_child = false?

> 3. Measures
> 
> I could thought up three approaches for the problem.

Thanks for writing up multiple options.  My tentative preference is for (3),
then (2), then (1):

> One is to simplly modifying here to give child flag in the
> parameters of add_eq_member accordig to whether the child_rel is
> inh or not. This seems to work fine although leaves two levels of
> MergeAppends (PATCH-1). So the additional patch is attached to
> collapse these MergeAppends (PATCH-2). This gives the same plan
> as PATCH-3.

Revising the append graph this late in planning (create_merge_append_path())
seems like the wrong location.  Changing add_child_rel_equivalences() in this
way adds further subtlety to the meaning of em_is_child, and I haven't
comprehended the full implications.  Neither of those objections are fatal,
but let's focus on the other two approaches, which avoid those objections.

> The second is to collapse the appendrel structure shown above to
> have only single level inheritance. This also seems to work
> fine. This makes the plan with single level
> MergeAppend. (PATCH-3)

> The last one is most strait-forward in some sense, and conversely
> is most ad hoc measure. Modifying expand_inherited_rtentry() to
> pull up adding appendrels if needed, using the similar manner to
> PATCH-3. This is added as PATCH-4.

The choice between (2) and (3) should depend on the extent of ways in which we
can end up with a nested AppendRelInfo graph.  If expand_inherited_rtentry()
is the only place that can introduce nesting, modifying it to stop doing that
makes sense.  Otherwise, adding a generic collapse_appendrels() step might
help more situations.  expand_inherited_rtentry() is the only culprit I can
identify, so I lean toward approach-3/PATCH-4.  For a data point corroborating
that hypothesis, "make check" issues no queries that trigger this warni

[HACKERS] COPY TO

[mohsen soodkhah mohammadi]

hello.
in copy.c is one function that its name is CopyOneRowTO.
in this function have one foreach loop. in this loop first if have this
condition: !cstate->binary
what means this condition and when true this condition?
thank you.

[HACKERS] Re: Server is not getting started with log level as debug5 on master after commit 3147ac

[Amit Kapila]

On Fri, Nov 22, 2013 at 9:30 AM, Amit Kapila  wrote:
> In master branch, server is not getting started with log level as debug5.
>
> Simple steps to reproduce the problem:
> a. initdb -D ..\..\Data
> b. change log_min_messages = debug5 in postgresql.conf
> c. start server (pg_ctl start -D ..\..\Data)  -- server doesn't get started
>
> Relevant message on server console:
> DEBUG:  mapped win32 error code 2 to 2
> FATAL:  could not open recovery command file "recovery.conf": No error
>
> This problem occurs only in Windows, but the cause of problem is generic.
>
> I could think of below possible ways to fix the problem:
> a. in function pvsnprintf(), save the errno before setting it to 0 and
> then before exiting function reset it back to saved errno. I think
> this is sane because in function pvsnprintf, if any error occurs due
> to which errno is changed, it will not return control, so errno will
> not be required by callers.
> b. we can change the callers like _dosmaperr() who have responsibility
> to save errno for their callers.
>
> Patch with approach a) is attached with mail, we can change code as
> per approach b) or any other better method as well, but for now I have
> prepared patch with approach a), as I could not see any problem with
> it.

Again looking at it, I think better fix would be to restore 'errno'
from 'edata->saved_errno' in errfinish() incase the control returns
back to caller (elevel <= WARNING). It seems to me that this fix is
required anyway because if we return from errfinish (ereport/elog) to
caller, it should restore back 'errno', as caller might need to take
some action based on errno.
Patch to restore 'errno' in errfinish() is attached with this mail.

Any better ideas for fixing this issue?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


pvsnprintf_issue_v2.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] Proof of concept: standalone backend with full FE/BE protocol

[Amit Kapila]

On Thu, Nov 21, 2013 at 9:54 PM, Amit Kapila  wrote:
> On Thu, Nov 21, 2013 at 8:14 PM, Tom Lane  wrote:
>> Amit Kapila  writes:
>>> Here what I have in mind is that:
>>
>> Why would you make psql behave differently from our other command-line
>> clients?
>
>No, psql should not behave different from other clients. Sorry, I
> was under assumption that for other programs we will not take backend
> executable
>path.
>One other thing which is not clear to me is that how by calling
> some special/new API we can ensure that the path provided by user is
>a valid path, are we going to validate file given in
> 'standalone_backend'  switch in some way?

Here, do we mean that if user specifies special switch, then
psql/pg_dump will call new API (eg. PQstartSingleUser(dsn))  which
will use postgres from same bin directory they are in, rather than
using from standalone_backend.
Can we consider this as a way to proceed for this patch?

On a side note, today while reading what other software's does to
protect them from such a security threat, I came across this link
(http://osxbook.com/book/bonus/chapter7/binaryprotection/index.html)
which suggests to have encrypted binaries. The use for encrypted
binaries is somewhat similar to what we discussed as a security threat
in this mail thread. Some text from link which made me think that
this is relevant.
For example, "one could turn the requirement around and say that a
given system must not run any binaries unless they are from a
certain source (or set of sources). This could be used to create an
admission-control mechanism for executables, which in turn could
be used in defending against malware. In a draconian managed
environment, it might be desired to limit program execution on managed
systems to a predefined set of programs—nothing else will execute."

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


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

Re: [HACKERS] Building on S390

[Tom Lane]

Greg Stark  writes:
> On Fri, Nov 22, 2013 at 8:51 PM, Peter Eisentraut  wrote:
>> According to the Debian build logs, postgres-xc compiles the entire
>> backend with -fPIC.  Not sure what sense that makes.

> Debian policy is to always use -fPIC

> IIRC -fpic is good enough as long as the total size of the library is below
> some limit. I'm not sure precisely what this size is that has to be below
> the limit but if I recall correctly it's something you have no way to
> determine in advance for a general purpose library. So Debian decided long
> long ago to just use -fPIC always.

Well, in that case they did a really crappy job of applying that policy to
their Postgres packages, because it sure sounds like there's still some
-fpic switches laying about in their builds.  But in any case, that
seems to me like a pretty brain-dead policy (hint: if you need -fPIC,
you'll get build failures that tell you so), so I feel no need to adopt
it for standard Postgres builds.

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] Shave a few instructions from child-process startup sequence

[Gurjeet Singh]

On Wed, Nov 20, 2013 at 10:21 AM, Peter Eisentraut  wrote:

> On 11/5/13, 2:47 AM, Gurjeet Singh wrote:
> > On Mon, Nov 4, 2013 at 12:20 AM, Tom Lane  > > wrote:
> >
> > But we're not buying much.  A few instructions during postmaster
> > shutdown
> > is entirely negligible.
> >
> >
> > The patch is for ClosePostmasterPorts(), which is called from every
> > child process startup sequence (as $subject also implies), not in
> > postmaster shutdown. I hope that adds some weight to the argument.
>
> If there is a concern about future maintenance, you could add assertions
> (in appropriate compile mode) that the rest of the array is indeed
> PGINVALID_SOCKET.  I think that could be a win for both performance and
> maintainability.
>

Makes sense! Does the attached patch look like what you expected? I also
added a comment to explain the expectation.

Thanks and best regards,
-- 
Gurjeet Singh http://gurjeet.singh.im/

EDB Inc. www.EnterpriseDB.com 
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index ccb8b86..9efc9fa 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -2236,6 +2236,20 @@ ClosePostmasterPorts(bool am_syslogger)
 			StreamClose(ListenSocket[i]);
 			ListenSocket[i] = PGINVALID_SOCKET;
 		}
+else
+		{
+			/*
+			 * Do not process the rest of the array elements since we expect
+			 * the presence of an invalid socket id to mark the end of valid
+			 * elements.
+			 */
+#ifdef USE_ASSERT_CHECKING
+			int j;
+			for(j = i; j < MAXLISTEN; j++)
+Assert(ListenSocket[i] == PGINVALID_SOCKET);
+#endif
+break;
+		}
 	}
 
 	/* If using syslogger, close the read side of the pipe */

-- 
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] [GENERAL] pg_upgrade ?deficiency

[Kevin Grittner]

Andres Freund  wrote:
> On 2013-11-22 13:34:18 -0800, Kevin Grittner wrote:
>> Oddly, it didn't complain about creating users within a read-only
>> transaction.  That seems like a potential bug.
>
> There's lots of things that escape XactReadOnly. I've thought (and I
> think suggested) before that we should put in another layer of defense
> by also putting a check in AssignTransactionId(). Imo the compatibility
> woes (like not being able to run SELECT txid_current();) are well worth
> the nearly ironclad guarantee that we're not writing.

I agree that something like that is would be a good idea; however,
I'm sure you would agree that would not be material for a
back-patch to a stable branch.

Another thing I've mused about is having some way to lock a
database to read-only, such that only the owner or a superuser
could change that.  Another setting which I know some people would
like to lock is transaction isolation level.  I haven't really
thought of a good UI for that sort of thing, though.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency

[Kevin Grittner]

Bruce Momjian  wrote:
> On Fri, Nov 22, 2013 at 01:55:10PM -0800, Kevin Grittner wrote:
>
>> It does nothing about pg_upgrade, which is sort of a separate
>> issue.  My inclination is that connections to the new cluster
>> should set this and connections to the old should not.
>
> It is going to be tricky to conditionally set/reset an
> environment variable for default_transaction_read_only for
> old/new clusters.

Why?  If you know you have connected to the new cluster, set it to
off; if you know you have connected to the old cluster, don't touch
it.

> We never write to the old cluster, so I am not sure
> setting/resetting default_transaction_read_only is needed.

I'm sure it isn't.  That's why I said that connections to the old
cluster should not set it -- by which I meant they should not do
anything with it.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Can we trust fsync?

[Michael Paquier]

On Sat, Nov 23, 2013 at 8:06 AM, Peter Geoghegan  wrote:
> On Fri, Nov 22, 2013 at 2:57 PM, Bruce Momjian  wrote:
>> The program is diskchecker:
>>
>> http://brad.livejournal.com/2116715.html
>>
>> I got the author to re-host the source code on github a few years ago.
>
> It might be worth re-implementing this for -contrib. The fact that we
> mention diskchecker.pl in the docs, and it is a pretty obscure Perl
> script on some guy's personal website doesn't inspire much confidence.
Yes, having that in contrib would be useful. Those would bring a plus
when testing disks for Postgres.
-- 
Michael


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

Re: [HACKERS] Building on S390

[Greg Stark]

On Fri, Nov 22, 2013 at 8:51 PM, Peter Eisentraut  wrote:

> On 11/22/13, 12:41 PM, Michael Meskes wrote:
> > Checking the Debian logs it appears that all calls use *both* which
> seems to do
> > the right thing. And yes, it appears there is a change in XC that makes
> it
> > break. But still, I would think there has to be a correct set of options.
>
> According to the Debian build logs, postgres-xc compiles the entire
> backend with -fPIC.  Not sure what sense that makes.


Debian policy is to always use -fPIC

IIRC -fpic is good enough as long as the total size of the library is below
some limit. I'm not sure precisely what this size is that has to be below
the limit but if I recall correctly it's something you have no way to
determine in advance for a general purpose library. So Debian decided long
long ago to just use -fPIC always.


-- 
greg

Re: [HACKERS] Can we trust fsync?

[Bruce Momjian]

On Fri, Nov 22, 2013 at 03:27:29PM -0800, Josh Berkus wrote:
> On 11/22/2013 03:23 PM, Bruce Momjian wrote:
> > On Fri, Nov 22, 2013 at 03:06:31PM -0800, Peter Geoghegan wrote:
> >> On Fri, Nov 22, 2013 at 2:57 PM, Bruce Momjian  wrote:
> >>> The program is diskchecker:
> >>>
> >>> http://brad.livejournal.com/2116715.html
> >>>
> >>> I got the author to re-host the source code on github a few years ago.
> >>
> >> It might be worth re-implementing this for -contrib. The fact that we
> >> mention diskchecker.pl in the docs, and it is a pretty obscure Perl
> >> script on some guy's personal website doesn't inspire much confidence.
> > 
> > Well, it was his idea, and quite a good one.  I guess we could
> > reimplement this in C if someone wants to do the legwork.
> 
> Yeah, too bad Brad didn't post a license for it.

We can ask him.

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

  + Everyone has their own god. +


-- 
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] Can we trust fsync?

[Josh Berkus]

On 11/22/2013 03:23 PM, Bruce Momjian wrote:
> On Fri, Nov 22, 2013 at 03:06:31PM -0800, Peter Geoghegan wrote:
>> On Fri, Nov 22, 2013 at 2:57 PM, Bruce Momjian  wrote:
>>> The program is diskchecker:
>>>
>>> http://brad.livejournal.com/2116715.html
>>>
>>> I got the author to re-host the source code on github a few years ago.
>>
>> It might be worth re-implementing this for -contrib. The fact that we
>> mention diskchecker.pl in the docs, and it is a pretty obscure Perl
>> script on some guy's personal website doesn't inspire much confidence.
> 
> Well, it was his idea, and quite a good one.  I guess we could
> reimplement this in C if someone wants to do the legwork.

Yeah, too bad Brad didn't post a license for it.


-- 
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] Can we trust fsync?

[Bruce Momjian]

On Fri, Nov 22, 2013 at 03:06:31PM -0800, Peter Geoghegan wrote:
> On Fri, Nov 22, 2013 at 2:57 PM, Bruce Momjian  wrote:
> > The program is diskchecker:
> >
> > http://brad.livejournal.com/2116715.html
> >
> > I got the author to re-host the source code on github a few years ago.
> 
> It might be worth re-implementing this for -contrib. The fact that we
> mention diskchecker.pl in the docs, and it is a pretty obscure Perl
> script on some guy's personal website doesn't inspire much confidence.

Well, it was his idea, and quite a good one.  I guess we could
reimplement this in C if someone wants to do the legwork.

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

  + Everyone has their own god. +


-- 
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] [GENERAL] pg_upgrade ?deficiency

[Bruce Momjian]

On Fri, Nov 22, 2013 at 01:55:10PM -0800, Kevin Grittner wrote:
> It does nothing about pg_upgrade, which is sort of a separate
> issue.  My inclination is that connections to the new cluster
> should set this and connections to the old should not.

It is going to be tricky to conditionally set/reset an environment
variable for default_transaction_read_only for old/new clusters.  We
never write to the old cluster, so I am not sure setting/resetting
default_transaction_read_only is needed.

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

  + Everyone has their own god. +


-- 
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] Can we trust fsync?

[Peter Geoghegan]

On Fri, Nov 22, 2013 at 2:57 PM, Bruce Momjian  wrote:
> The program is diskchecker:
>
> http://brad.livejournal.com/2116715.html
>
> I got the author to re-host the source code on github a few years ago.

It might be worth re-implementing this for -contrib. The fact that we
mention diskchecker.pl in the docs, and it is a pretty obscure Perl
script on some guy's personal website doesn't inspire much confidence.

-- 
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] Can we trust fsync?

[Bruce Momjian]

On Fri, Nov 22, 2013 at 11:16:06AM -0500, Tom Lane wrote:
> Greg Stark  writes:
> > On Thu, Nov 21, 2013 at 1:43 AM, Tom Lane  wrote:
> >> Also, it's not that hard to do plug-pull testing to verify that your
> >> system is telling the truth about fsync.  This really ought to be part
> >> of acceptance testing for any new DB server.
> 
> > I've never tried it but I always wondered how easy it was to do. How would
> > you ever know you had tested it enough?
> 
> I used the program Greg Smith recommends on our wiki (can't remember the
> name offhand) when I got a new house server this spring.  With the RAID
> card configured for writethrough and no battery, it failed all over the
> place.  Fixed those configuration bugs, it was okay three or four times
> in a row, which was good enough for me.
> 
> > The original mail was referencing a problem with syncing *meta* data
> > though. The semantics around meta data syncs are much less clearly
> > specified, in part because file systems traditionally made nearly all meta
> > data operations synchronous. Doing plug-pull testing on Postgres would not
> > test meta data syncing very well since Postgres specifically avoids doing
> > much meta data operations by overwriting existing files and blocks as much
> > as possible.
> 
> True.  You're better off with a specialized testing program.  (Though
> now you mention it, I wonder whether that program was stressing metadata
> or not.)

The program is diskchecker:

http://brad.livejournal.com/2116715.html

I got the author to re-host the source code on github a few years ago.

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

  + Everyone has their own god. +


-- 
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 for fail-back without fresh backup

[Bruce Momjian]

On Thu, Nov 21, 2013 at 11:43:34PM +0100, Andres Freund wrote:
> On 2013-11-21 14:40:36 -0800, Jeff Janes wrote:
> > But if the transaction would not have otherwise generated WAL (i.e. a
> > select that did not have to do any HOT pruning, or an update with zero rows
> > matching the where condition), doesn't it now have to flush and wait when
> > it would otherwise not?
> 
> We short circuit that if there's no xid assigned. Check
> RecordTransactionCommit().

OK, that was my question, now answered.  Thanks.

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

  + Everyone has their own god. +


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

Re: [HACKERS] why semicolon after begin is not allowed in postgresql?

[Andrew Dunstan]

On 11/22/2013 05:24 PM, AK wrote:

I am reading the following in the documentation: "Tip: A common mistake is to
write a semicolon immediately after BEGIN. This is incorrect and will result
in a syntax error."

So, "common mistake" means semicolons after BEGIN seem consistent to many
people - it seems consistent to me as well. If PostgreSql allowed them, we
would have one less rule to memorize, shorter documentation, less mistakes
and so on. In other words, without this limitation PostgreSql would be
slightly more useful, right?

What am I missing? Why do we need this rule? How is it making PostgreSql
better?






You're referring specifically to plpgsql, not to Postgres or SQL generally.

plpgsql is derived from PLSQL which is derived from Ada which has this 
grammatical rule.


The explanation is this: only complete statements are followed by 
semicolons. But in these languages, "begin" on its own is not a complete 
statement. It's the start of a compound statement, the end of which will 
be "end", which is indeed followed by a semicolon.


cheers

andrew


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

Re: [HACKERS] why semicolon after begin is not allowed in postgresql?

[Kevin Grittner]

AK  wrote:

> I am reading the following in the documentation: "Tip: A common
> mistake is to write a semicolon immediately after BEGIN. This is
> incorrect and will result in a syntax error."
>
> So, "common mistake" means semicolons after BEGIN seem consistent
> to many people - it seems consistent to me as well. If PostgreSql
> allowed them, we would have one less rule to memorize, shorter
> documentation, less mistakes and so on. In other words, without
> this limitation PostgreSql would be slightly more useful, right?
>
> What am I missing? Why do we need this rule? How is it making
> PostgreSql better?

I think it only seems confusing because PostgreSQL also uses BEGIN
as a synonym for START TRANSACTION (and people tend to use the
shorter synonym to save keystrokes).  In plpgsql BEGIN is not a
command, it is part of declaring a code block.  Wouldn't these look
funny to you?:

IF x = 1 THEN;
  ...
END IF;

CASE;
  WHEN x = 1 THEN
    ...
  WHEN x = 2 THEN
    ...
  ELSE
    ...
END;

LOOP;
  ...
END LOOP;

etc.

Why should BEGIN be different from the above when it is not a
command, but part of declaring a code block?

In the nearest analog in the SQL standard, the BEGIN/END block is
called a compound statement, and like any other statement it is
ended by a semicolon; the standard does not allow a semicolon after
the BEGIN.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] why semicolon after begin is not allowed in postgresql?

[Merlin Moncure]

On Fri, Nov 22, 2013 at 4:34 PM, Mike Blackwell  wrote:
> I believe the section you are reading refers to the BEGIN keyword in the
> procedural language plpgsql, not the SQL 'BEGIN' command.  The issue stems
> from confusing two distinct languages both of which, along with several more
> procedural languages, are documented in the same manual.

This is inherited constraint from Oracle pl/sql which pl/pgsql is, uh,
inspired by.  In pl/sql, all block opening constructs (THEN, LOOP,
BEGIN) do not get semi-colons.  BEGIN is a weird case because it's
(quite unfortunately) also the same thing that explicitly opens a
transaction in vanilla SQL; you use a semi-colon there as with any
standard SQL statement.

merlin


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

Re: [HACKERS] why semicolon after begin is not allowed in postgresql?

[Mike Blackwell]

I believe the section you are reading refers to the BEGIN keyword in the
procedural language plpgsql, not the SQL 'BEGIN' command.  The issue stems
from confusing two distinct languages both of which, along with several
more procedural languages, are documented in the same manual.

__
*Mike Blackwell | Technical Analyst, Distribution Services/Rollout
Management | RR Donnelley*
1750 Wallace Ave | St Charles, IL 60174-3401
Office: 630.313.7818
mike.blackw...@rrd.com
http://www.rrdonnelley.com



* *


On Fri, Nov 22, 2013 at 4:24 PM, AK  wrote:

> I am reading the following in the documentation: "Tip: A common mistake is
> to
> write a semicolon immediately after BEGIN. This is incorrect and will
> result
> in a syntax error."
>
> So, "common mistake" means semicolons after BEGIN seem consistent to many
> people - it seems consistent to me as well. If PostgreSql allowed them, we
> would have one less rule to memorize, shorter documentation, less mistakes
> and so on. In other words, without this limitation PostgreSql would be
> slightly more useful, right?
>
> What am I missing? Why do we need this rule? How is it making PostgreSql
> better?
>
>
>
> --
> View this message in context:
> http://postgresql.1045698.n5.nabble.com/why-semicolon-after-begin-is-not-allowed-in-postgresql-tp5779905.html
> Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Re: [HACKERS] why semicolon after begin is not allowed in postgresql?

[Adrian Klaver]

On 11/22/2013 02:24 PM, AK wrote:

I am reading the following in the documentation: "Tip: A common mistake is to
write a semicolon immediately after BEGIN. This is incorrect and will result
in a syntax error."

So, "common mistake" means semicolons after BEGIN seem consistent to many
people - it seems consistent to me as well. If PostgreSql allowed them, we
would have one less rule to memorize, shorter documentation, less mistakes
and so on. In other words, without this limitation PostgreSql would be
slightly more useful, right?


In Postgresql it is allowed:

test=> BEGIN ;
BEGIN

In plpgsql it is not, which is where you got the above documentation. 
That is because SQL BEGIN != plpgsql BEGIN





What am I missing? Why do we need this rule? How is it making PostgreSql
better?



--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/why-semicolon-after-begin-is-not-allowed-in-postgresql-tp5779905.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.





--
Adrian Klaver
adrian.kla...@gmail.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] [GENERAL] pg_upgrade ?deficiency

[Kevin Grittner]

Andres Freund  wrote:

> FWIW, I am less than convinced that it is correct for
> pg_dump[all] to emit SET default_transaction_readonly = off;

It doesn't change the database setting, just the connection which
is issuing commands to create global object -- ones that don't
reside in the database we are connected to.  As the comment in
pg_dumpall.c says, right above where I added this:

    /*
 * We used to emit \connect postgres here, but that served no purpose
 * other than to break things for installations without a postgres
 * database.  Everything we're restoring here is a global, so whichever
 * database we're connected to at the moment is fine.
 */

> The user might explicitly have set that to prevent against
> somebody restoring into the wrong database or somesuch. Why is it
> our business to override such decisions?

Hmm.  If your argument had been that the user intended that the
database be a read-only database, then I would say that your
argument held no water.  Any user can choose to make any
transaction (or all subsequent transactions on the connection)
read-write any time they want.  The problem with pg_dumpall as it
stands is that it sets the databases to that default and then tries
to load data into them, which fails.

But you have a point regarding adding what I proposed to pg_dump. 
The more I think about it, the more I'm inclined to think that
pg_dumpall should insert this right after the \connect to a
database it is going to write to, rather than affecting pg_dump
output at all. That would allow you default protection against
writing pg_dump output to a database that was flagged this way. 
You could get around it by connecting interactively with psql,
issuing a SET command, and bringing in dump output with \i from a
text file.  If we go this way, would we want options on pg_restore
and/or psql to issue this set when reading in a file (or piped
input)?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

[HACKERS] why semicolon after begin is not allowed in postgresql?

[AK]

I am reading the following in the documentation: "Tip: A common mistake is to
write a semicolon immediately after BEGIN. This is incorrect and will result
in a syntax error."

So, "common mistake" means semicolons after BEGIN seem consistent to many
people - it seems consistent to me as well. If PostgreSql allowed them, we
would have one less rule to memorize, shorter documentation, less mistakes
and so on. In other words, without this limitation PostgreSql would be
slightly more useful, right?

What am I missing? Why do we need this rule? How is it making PostgreSql
better?



--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/why-semicolon-after-begin-is-not-allowed-in-postgresql-tp5779905.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.


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

Re: [HACKERS] Why is UPDATE with column-list syntax not implemented

[Claudio Freire]

On Fri, Nov 22, 2013 at 6:36 PM, AK  wrote:
> Claudio,
>
> Can you elaborate how rules can help?


Well... that specific example:

> UPDATE accounts SET (contact_last_name, contact_first_name) =
> (SELECT last_name, first_name FROM salesmen
>  WHERE salesmen.id = accounts.sales_id);

Can be rewritten as

UPDATE accounts SET contact_last_name = t.last_name,
contact_first-name = t.first_name
FROM (SELECT salesmen.id as salesmen_id, last_name, first_name FROM salesmen) t
WHERE t.salesmen_id = accounts.sales_id;

That's not 100% general, but it's quite general enough, transforming:

UPDATE  SET () = (SELECT  
WHERE . =  )

Into

UPDATE  SET  FROM (SELECT
 AS _,   WHERE
) tmp WHERE . = tmp._;

That's *almost* a regex.

It's possible the transformation can be done at the AST-level more
generally, but I don't know enough of postgres parser to go deeper
into that path, but the general idea being that it can be done even
more generally with CTEs, if the where clause terms that relate to the
updated table can be pinpointed and extracted into the CTE (as long as
they're stable).


-- 
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] [GENERAL] pg_upgrade ?deficiency

[Andres Freund]

On 2013-11-22 13:34:18 -0800, Kevin Grittner wrote:
> I changed my postgres database to default to read only (which is
> not insane, given that I've seen so many people accidentally run
> DDL there rather than in the database they intended)

FWIW, I am less than convinced that it is correct for pg_dump[all] to
emit SET default_transaction_readonly = off; The user might explicitly
have set that to prevent against somebody restoring into the wrong
database or somesuch. Why is it our business to override such decisions?

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] [GENERAL] pg_upgrade ?deficiency

[Kevin Grittner]

Kevin Grittner  wrote:

This covers pg_dumpall globals.  Tested with a read-only postgres
database and with default_transaction_read_only = on in the
postgresql.conf file.

It does nothing about pg_upgrade, which is sort of a separate
issue.  My inclination is that connections to the new cluster
should set this and connections to the old should not.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Companydiff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 63a8009..199ffb0 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -2586,6 +2586,9 @@ _doSetFixedOutputState(ArchiveHandle *AH)
 	/* Likewise for lock_timeout */
 	ahprintf(AH, "SET lock_timeout = 0;\n");
 
+	/* Restore will need to write to the target database */
+	ahprintf(AH, "SET default_transaction_read_only = off;\n");
+
 	/* Select the correct character set encoding */
 	ahprintf(AH, "SET client_encoding = '%s';\n",
 			 pg_encoding_to_char(AH->public.encoding));
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 336ae58..4540a19 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -452,6 +452,9 @@ main(int argc, char *argv[])
 	 * database we're connected to at the moment is fine.
 	 */
 
+	/* Restore will need to write to the target database */
+	fprintf(OPF, "SET default_transaction_read_only = off;\n");
+
 	/* Replicate encoding and std_strings in output */
 	fprintf(OPF, "SET client_encoding = '%s';\n",
 			pg_encoding_to_char(encoding));

-- 
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] [GENERAL] pg_upgrade ?deficiency

[Andres Freund]

On 2013-11-22 13:34:18 -0800, Kevin Grittner wrote:
> Oddly, it didn't complain about creating users within a read-only
> transaction.  That seems like a potential bug.

There's lots of things that escape XactReadOnly. I've thought (and I
think suggested) before that we should put in another layer of defense
by also putting a check in AssignTransactionId(). Imo the compatibility
woes (like not being able to run SELECT txid_current();) are well worth
the nearly ironclad guarantee that we're not writing.

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] Why is UPDATE with column-list syntax not implemented

[AK]

Thank you, Tom!



--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/Why-is-UPDATE-with-column-list-syntax-not-implemented-tp5779600p5779899.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.


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

Re: [HACKERS] Why is UPDATE with column-list syntax not implemented

[AK]

Claudio,

Can you elaborate how rules can help?



--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/Why-is-UPDATE-with-column-list-syntax-not-implemented-tp5779600p5779896.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.


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

Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency

[Kevin Grittner]

Andres Freund  wrote:
> On 2013-11-22 13:07:29 -0800, Kevin Grittner wrote:
>> Andres Freund  wrote:
>>
>>> are you sure it also unsets default_transaction_readonly for
>>> the roles etc. it creates?
>>
>> I'm not sure I understand.  Could you give an example of what
>> you're concerned about?
>
> pg_dumpall first spits out global data (users, databases, tablespaces)
> and then invokes pg_dump for every single database. So I'd very strongly
> suspect that your patch will issue the CREATE ROLE etc. calls without
> unsetting default_transaction_readonly.

I changed my postgres database to default to read only (which is
not insane, given that I've seen so many people accidentally run
DDL there rather than in the database they intended), and got these
errors when I used it for my connection to restore pg_dumpall
output:

ERROR:  cannot execute COMMENT in a read-only transaction
ERROR:  cannot execute CREATE EXTENSION in a read-only transaction
ERROR:  cannot execute COMMENT in a read-only transaction
ERROR:  cannot execute REVOKE in a read-only transaction
ERROR:  cannot execute REVOKE in a read-only transaction
ERROR:  cannot execute GRANT in a read-only transaction
ERROR:  cannot execute GRANT in a read-only transaction

Oddly, it didn't complain about creating users within a read-only
transaction.  That seems like a potential bug.

Will look at covering pg_dumpall for that initial connection.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency

[Tom Lane]

Andres Freund  writes:
> On 2013-11-22 13:07:29 -0800, Kevin Grittner wrote:
>> I'm not sure I understand.  Could you give an example of what
>> you're concerned about?

> pg_dumpall first spits out global data (users, databases, tablespaces)
> and then invokes pg_dump for every single database. So I'd very strongly
> suspect that your patch will issue the CREATE ROLE etc. calls without
> unsetting default_transaction_readonly.

Yeah, that's what I was wondering about.  I don't think pg_dumpall -g
invokes pg_dump at all, so how could that case work?  Maybe it would
only fail if the postgres database is read-only, though.  Try it with
default-read-only set in postgresql.conf instead of as a database
property.

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] [GENERAL] pg_upgrade ?deficiency

[Andres Freund]

On 2013-11-22 13:07:29 -0800, Kevin Grittner wrote:
> Andres Freund  wrote:
> 
> > are you sure it also unsets default_transaction_readonly for
> > the roles etc. it creates?
> 
> I'm not sure I understand.  Could you give an example of what
> you're concerned about?

pg_dumpall first spits out global data (users, databases, tablespaces)
and then invokes pg_dump for every single database. So I'd very strongly
suspect that your patch will issue the CREATE ROLE etc. calls without
unsetting default_transaction_readonly.

E.g. output looks like:
--
-- PostgreSQL database cluster dump
--
..
CREATE ROLE andres;
ALTER ROLE andres WITH SUPERUSER INHERIT CREATEROLE CREATEDB LOGIN
REPLICATION;
...
\connect postgres

--
-- PostgreSQL database dump
--


CREATE TABLE pgbench_accounts (
aid integer NOT NULL,
bid integer,
abalance integer,
filler character(84)
)
WITH (fillfactor=100);

...
\connect regression
...


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] [GENERAL] pg_upgrade ?deficiency

[Kevin Grittner]

Andres Freund  wrote:

> are you sure it also unsets default_transaction_readonly for
> the roles etc. it creates?

I'm not sure I understand.  Could you give an example of what
you're concerned about?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency

[Andres Freund]

On 2013-11-22 12:45:25 -0800, Kevin Grittner wrote:
> Tom Lane  wrote:
> 
> > That looks sane for pg_dump, but I would rather have expected
> > that pg_dumpall would need to emit the same thing (possibly more
> > than once due to reconnections).
> 
> I was kinda surprised myself.  I changed it for pg_dump, tested
> that, and then tested pg_dumpall to get a baseline, and the setting
> was taken care of.

pg_dumpall is lazy and just executes pg_dump for every database, that's
the reason... But are you sure it also unsets default_transaction_readonly for
the roles etc. it creates?

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] Building on S390

[Peter Eisentraut]

On 11/22/13, 12:41 PM, Michael Meskes wrote:
> Checking the Debian logs it appears that all calls use *both* which seems to 
> do
> the right thing. And yes, it appears there is a change in XC that makes it
> break. But still, I would think there has to be a correct set of options.

According to the Debian build logs, postgres-xc compiles the entire
backend with -fPIC.  Not sure what sense that makes.



-- 
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] [GENERAL] pg_upgrade ?deficiency

[Kevin Grittner]

Tom Lane  wrote:

> That looks sane for pg_dump, but I would rather have expected
> that pg_dumpall would need to emit the same thing (possibly more
> than once due to reconnections).

I was kinda surprised myself.  I changed it for pg_dump, tested
that, and then tested pg_dumpall to get a baseline, and the setting
was taken care of.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency

[Tom Lane]

Kevin Grittner  writes:
> Kevin Grittner  wrote:
>> See the attached patch.

> Trying that again.

That looks sane for pg_dump, but I would rather have expected that
pg_dumpall would need to emit the same thing (possibly more than once
due to reconnections).

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] [GENERAL] pg_upgrade ?deficiency

[Kevin Grittner]

Kevin Grittner  wrote:

> See the attached patch.

Trying that again.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Companydiff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 63a8009..199ffb0 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -2586,6 +2586,9 @@ _doSetFixedOutputState(ArchiveHandle *AH)
 	/* Likewise for lock_timeout */
 	ahprintf(AH, "SET lock_timeout = 0;\n");
 
+	/* Restore will need to write to the target database */
+	ahprintf(AH, "SET default_transaction_read_only = off;\n");
+
 	/* Select the correct character set encoding */
 	ahprintf(AH, "SET client_encoding = '%s';\n",
 			 pg_encoding_to_char(AH->public.encoding));

-- 
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] Building on S390

[Tom Lane]

Michael Meskes  writes:
> On Fri, Nov 22, 2013 at 11:27:45AM -0500, Tom Lane wrote:
>> Furthermore, if we change that convention now, we're going to increase
>> the risk of such mixing failures for other people.

> Sure, but if this a bug we should. I'm not saying it is, I simply don't know.

Well, *if* it's a bug in core PG then we should do something about it,
but at the moment there's no evidence of that.  What seems the most
likely theory here is that the Debian maintainer has broken their package
with an ill-considered patch.  We can't take responsibility for other
people's hacks.

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] Why is UPDATE with column-list syntax not implemented

[Tom Lane]

AK  writes:
> 9.3 documentation says:
> According to the standard, the column-list syntax should allow a list of
> columns to be assigned from a single row-valued expression, such as a
> sub-select:

> UPDATE accounts SET (contact_last_name, contact_first_name) =
> (SELECT last_name, first_name FROM salesmen
>  WHERE salesmen.id = accounts.sales_id);
> This is not currently implemented — the source must be a list of independent
> expressions.

> Why is this not implemented? Is it considered inconvenient to use, or
> difficult to implement. or not important enough, or some other reason?

It's difficult to implement.  You'd need to do some significant
restructuring of the way UPDATE is handled.  Probably someone will
attempt it at some point.

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] [GENERAL] pg_upgrade ?deficiency

[Kevin Grittner]

Bruce Momjian  wrote:

>> Not sure about backpatching.  default_transaction_read_only has been
>> around since 7.4.  Setting it to true would cause pg_dump to fail unless
>> you changed the database setting, and pg_dumpall would fail completely
>> as there is no way to turn off the database setting.

See the attached patch.  It seems to fix pg_dump and pg_dumpall.  I
don't think it will cause any problem for *dumping* earlier
versions,  Backpatching would mean that if you try to restore a
dump made by 8.4 or later software to a 7.3 or earlier database,
you would get an error; but I don't think that's supported, and I
would be amazed if that were the *only* error you got if you tried
that.

>> The problem is that I don't remember any report of this failing in
>> pg_dump, pg_dumpall, or pg_upgrade, so saying it is a major issue is
>> hard to accept.

Any time that you can appear to successfully dump a database, and
the restore attempt fails, I consider that to be a major issue.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Replication Node Identifiers and crashsafe Apply Progress

[Andres Freund]

On 2013-11-22 14:43:15 -0500, Robert Haas wrote:
> The patch as proposed puts forward a particular way of
> doing that, and I think that neither that method *nor any other*
> should be part of core.

Working on something like that, updated the patch state to "waiting on
author".

Thanks,

Andres Freund

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


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

Re: [HACKERS] UNNEST with multiple args, and TABLE with multiple funcs

[Tom Lane]

Andrew Gierth  writes:
> "Tom" == Tom Lane  writes:
>  Tom> Well, it's not insane on its face.  The rowtype of f in the
>  Tom> first example is necessarily a built-on-the-fly record, but in
>  Tom> the second case using the properties of the underlying named
>  Tom> composite type is possible, and consistent with what happens in
>  Tom> 9.3 and earlier.  (Not that I'm claiming we were or are totally
>  Tom> consistent ...)

> Right, but your changes to the code make it look like there was an
> intended change there - with the scan type tupdesc being forced to
> RECORD type and its column names changed.

I did set things up so that if you have a RECORD result, the column names
will be those of the query's alias list; this was in response to the
comment in the patch that complained that we were inconsistent about where
we were getting the names from if you had a mix of named-composite
functions and other functions.  I believe what is happening in the case
you show is that the function is returning a composite Datum that's marked
with the composite type's OID, and the upstream consumers are looking at
that, not at the scan tupdesc.  I'm not really excited about tracing down
exactly what the data flow is ...

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] Replication Node Identifiers and crashsafe Apply Progress

[Robert Haas]

On Thu, Nov 21, 2013 at 8:26 AM, Andres Freund  wrote:
> On 2013-11-21 08:22:05 -0500, Robert Haas wrote:
>> On Thu, Nov 21, 2013 at 6:15 AM, Andres Freund  
>> wrote:
>> >> > WRT performance: I agree that fixed-width identifiers are more
>> >> > performant, that's why I went for them, but I am not sure it's that
>> >> > important. The performance sensitive parts should all be done using the
>> >> > internal id the identifier maps to, not the public one.
>> >>
>> >> But I thought the internal identifier was exactly what we're creating.
>> >
>> > Sure. But how often are we a) going to create such an identifier b)
>> > looking it up?
>>
>> Never.  Make that the replication solution's problem.  Make the core
>> support deal only with UUIDs or pairs of 64-bit integers or something
>> like that, and let the replication solution decide what they mean.
>
> I think we're misunderstanding each other. I was commenting on your fear
> that strings longer than NAMEDATALEN or something would be bad for
> performance - which I don't think is very relevant because the lookups
> from "public" to "internal" identifier shouldn't be in any performance
> critical path.
>
> I personally would prefer a string because it'd allow me to build an
> identifier using the criterions I'd originally outlined outside of this
> infrastructure.

Yeah, there's some confusion here.  I don't care at all about the
performance characteristics of long strings here, because we shouldn't
be using them anywhere in the core code.  What I do care about is
making sure that whatever core support we use here is agnostic to how
the internal identifiers - relatively short bit strings - are
generated.  The patch as proposed puts forward a particular way of
doing that, and I think that neither that method *nor any other*
should be part of core.

-- 
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] new unicode table border styles for psql

[Pavel Stehule]

2013/11/22 Alvaro Herrera 

> Pavel Stehule escribió:
>
> > 2013/11/21 Peter Eisentraut 
>
> > > Maybe make the border setting a string containing the various
> characters
> > > by index.  Then everyone can create their own crazy borders.
> > >
> > I seriously though about it, but not sure if it is good way.
>
> How about having a single "unicode" line style, and then have a
> different \pset setting to determine exactly what chars to print?  This
> wouldn't allow for programmability, but it seems better UI to me.
> This proliferation of unicode line style names seems odd.
>

-1

After thinking I don't see any value for common user. Users like you, me,
Merlin are able to parametrize output or patching source code.

Any parametrization expect some secure store, that will support exchange of
styles. And it expect robust parser of unicode strings, or ascii strings
with unicode escaped chars.

We cannot parse a escaped unicode chars now on client side, and cost of
parser is higher of benefit externally parametrized borders.

Regards

Pavel


>
> --
> Įlvaro Herrerahttp://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>

Re: [HACKERS] MultiXact pessmization in 9.3

[Alvaro Herrera]

Andres Freund wrote:
> On 2013-11-22 12:04:31 -0300, Alvaro Herrera wrote:

> Yes, somewhat: <9.3 GetMultiXactIdMembers() didn't do any work if the
> multixact was old enough:
>   if (MultiXactIdPrecedes(multi, OldestVisibleMXactId[MyBackendId]))
>   {
>   debug_elog2(DEBUG2, "GetMembers: it's too old");
>   *xids = NULL;
>   return -1;
>   }
> so, in OLTP style workloads, doing a heap_lock_tuple() often didn't have
> to perform any IO when locking a tuple that previously had been locked
> using a multixact. We knew that none of the members could still be
> running and thus didn't have to ask the SLRU for them since a
> not-running member cannot still have a row locked.

Right.

> Now, fkey locks changed that because for !HEAP_XMAX_LOCK_ONLY mxacts, we
> need to look up the members to get the update xid and check whether it
> has committed or aborted, even if we know that it isn't currently
> running anymore due do OldestVisibleMXactId. But there's absolutely no
> need to do that for HEAP_XMAX_LOCK_ONLY mxacts, the old reasoning for
> not looking up the members if old is just fine.

Correct.  The only difficulty here is that we would need to pass down
the fact that we know for certain that this is only a locking Multixact.
There are some callers that go to it indirectly via MultiXactIdWait or
MultiXactIdExpand, but now that I look I think it's fine for those to
pass false (i.e. assume there might be an update and disable the
optimization), since those aren't hot compared to the other cases.

This patch implements this idea, but I haven't tested it much beyond
compiling and ensuring it passes the existing tests.

Regarding the outdated comment, I had a rewritten version of that
somewhere which I evidently forgot to push :-(  I noticed it was
outdated a couple of weeks after I pushed the main patch.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
*** a/contrib/pgrowlocks/pgrowlocks.c
--- b/contrib/pgrowlocks/pgrowlocks.c
***
*** 168,174  pgrowlocks(PG_FUNCTION_ARGS)
  
  allow_old = !(infomask & HEAP_LOCK_MASK) &&
  	(infomask & HEAP_XMAX_LOCK_ONLY);
! nmembers = GetMultiXactIdMembers(xmax, &members, allow_old);
  if (nmembers == -1)
  {
  	values[Atnum_xids] = "{0}";
--- 168,175 
  
  allow_old = !(infomask & HEAP_LOCK_MASK) &&
  	(infomask & HEAP_XMAX_LOCK_ONLY);
! nmembers = GetMultiXactIdMembers(xmax, &members, allow_old,
!  false);
  if (nmembers == -1)
  {
  	values[Atnum_xids] = "{0}";
*** a/src/backend/access/heap/heapam.c
--- b/src/backend/access/heap/heapam.c
***
*** 3987,3993  l3:
  			 * the case, HeapTupleSatisfiesUpdate would have returned
  			 * MayBeUpdated and we wouldn't be here.
  			 */
! 			nmembers = GetMultiXactIdMembers(xwait, &members, false);
  
  			for (i = 0; i < nmembers; i++)
  			{
--- 3987,3995 
  			 * the case, HeapTupleSatisfiesUpdate would have returned
  			 * MayBeUpdated and we wouldn't be here.
  			 */
! 			nmembers =
! GetMultiXactIdMembers(xwait, &members, false,
! 	  HEAP_XMAX_IS_LOCKED_ONLY(infomask));
  
  			for (i = 0; i < nmembers; i++)
  			{
***
*** 4008,4014  l3:
  }
  			}
  
! 			pfree(members);
  		}
  
  		/*
--- 4010,4017 
  }
  			}
  
! 			if (members)
! pfree(members);
  		}
  
  		/*
***
*** 4157,4163  l3:
   * been the case, HeapTupleSatisfiesUpdate would have returned
   * MayBeUpdated and we wouldn't be here.
   */
! nmembers = GetMultiXactIdMembers(xwait, &members, false);
  
  if (nmembers <= 0)
  {
--- 4160,4168 
   * been the case, HeapTupleSatisfiesUpdate would have returned
   * MayBeUpdated and we wouldn't be here.
   */
! nmembers =
! 	GetMultiXactIdMembers(xwait, &members, false,
! 		  HEAP_XMAX_IS_LOCKED_ONLY(infomask));
  
  if (nmembers <= 0)
  {
***
*** 5217,5223  GetMultiXactIdHintBits(MultiXactId multi, uint16 *new_infomask,
  	 * We only use this in multis we just created, so they cannot be values
  	 * pre-pg_upgrade.
  	 */
! 	nmembers = GetMultiXactIdMembers(multi, &members, false);
  
  	for (i = 0; i < nmembers; i++)
  	{
--- 5222,5228 
  	 * We only use this in multis we just created, so they cannot be values
  	 * pre-pg_upgrade.
  	 */
! 	nmembers = GetMultiXactIdMembers(multi, &members, false, false);
  
  	for (i = 0; i < nmembers; i++)
  	{
***
*** 5293,5299  MultiXactIdGetUpdateXid(TransactionId xmax, uint16 t_infomask)
  	 * Since we know the LOCK_ONLY bit is not set, this cannot be a multi from
  	 * pre-pg_upgrade.
  	 */
! 	nmembers = GetMultiXactIdMembers(xmax, &members, false);
  
  	if (nmembers > 0)
  	{
--- 5298,5304 
  	 * Since we know the LOCK_ONLY bit is not set, this cannot be a multi from
  	 * pre-pg_upgrade.
  	 */
! 	nme

Re: [HACKERS] Status of FDW pushdowns

[David Fetter]

On Fri, Nov 22, 2013 at 08:25:05AM -0600, Merlin Moncure wrote:
> On Thu, Nov 21, 2013 at 6:43 PM, Shigeru Hanada
>  wrote:
> > 2013/11/22 Tom Lane :
> >> Merlin Moncure  writes:
> >>> On Thu, Nov 21, 2013 at 9:05 AM, Bruce Momjian  wrote:
>  I know join pushdowns seem insignificant, but it helps to restrict what
>  data must be passed back because you would only pass back joined rows.
> >>
> >>> By 'insignificant' you mean 'necessary to do any non-trivial real
> >>> work'.   Personally, I'd prefer it if FDW was extended to allow
> >>> arbitrary parameterized queries like every other database connectivity
> >>> API ever made ever.
> >>
> >> [ shrug... ]  So use dblink.  For better or worse, the FDW stuff is
> >> following the SQL standard's SQL/MED design, which does not do it
> >> like that.
> >
> > Pass-through mode mentioned in SQL/MED standard might be what he wants.
> 
> happen to have a link handy?

http://www.wiscorp.com/sql20nn.zip

You'll want to look at the PDF with MED in its title.

Passthrough mode, which is how the standard "handles" this problem is
basically a thing where you set it to be on, then everything your send
until setting it to off is passed through to the remote side.  The
people writing the standard didn't think too much about the
possibility that the remote side might speak a broader or different
dialect of SQL from the local server.  They also didn't imagine cases
where what's being passed isn't SQL at all.

In addition to breaking any possible parser, the "feature" as
described in the standard is just ripe for un-patchable exploits *in
its design*.

Of all the misdesign-by-committee contained in the standard, this
piece is far and away the stupidest I've encountered to date.  We
should not even vaguely attempt to implement it.

Cheers,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


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

Re: [HACKERS] COPY table FROM STDIN doesn't show count tag

[Rajeev rastogi]

On 21 November 2013, Amit Khandekar 
mailto:amit.khande...@enterprisedb.com>> wrote:
>Ok. we will then first fix the \COPY TO issue where it does not revert back 
>the overriden psql output file handle. Once this is solved, fix for both COPY 
>FROM and COPY TO, like how it is done in the patch earlier 
>(copydefectV2.patch).

I analyzed the solution to fix \COPY TO issue but unfortunately I observed that 
do_copy is already resetting the value of cur_cmd_source and queryFout but 
before that itself result status is printed. So we'll have to reset the value 
before result status is being displayed.

So as other alternative solutions, I have two approaches:

1.  We can store current file destination queryFout in some local variable 
and pass the same to SendQuery function as a parameter. Same can be used to 
reset the value of queryFout after return from ProcessResult

>From all other callers of SendQuery , we can pass NULL value for this new 
>parameter.

2.  We can add new structure member variable FILE *prevQueryFout in 
structure "struct _psqlSettings", which hold the value of queryFout before 
being changed in do_copy. And then same can be used to reset value in SendQuery 
or ProcessResult.

Please let me know which approach is OK or if any other approach suggested.
Based on feedback I shall prepare the new patch and share the same.

Thanks and Regards,
Kumar Rajeev Rastogi

[HACKERS] Why is UPDATE with column-list syntax not implemented

[AK]

9.3 documentation says:

According to the standard, the column-list syntax should allow a list of
columns to be assigned from a single row-valued expression, such as a
sub-select:

UPDATE accounts SET (contact_last_name, contact_first_name) =
(SELECT last_name, first_name FROM salesmen
 WHERE salesmen.id = accounts.sales_id);
This is not currently implemented — the source must be a list of independent
expressions.

Why is this not implemented? Is it considered inconvenient to use, or
difficult to implement. or not important enough, or some other reason?



--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/Why-is-UPDATE-with-column-list-syntax-not-implemented-tp5779600.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.


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

Re: [HACKERS] Logging WAL when updating hintbit

[Dilip kumar]

On 20 November 2013 22:12, Sawada Masahiko Wrote

> >
> > 1. Patch applies cleanly to master HEAD.
> > 2. No Compilation Warning.
> > 3. It works as per the patch expectation.
> >
> > Some Suggestion:
> > 1. Add new WAL level ("all") in comment in postgresql.conf
> >wal_level = hot_standby # minimal, archive,
> or hot_standby
> 
> Thank you for reviewing the patch.
> Yes, I will do that. And I'm going to implement documentation patch.

OK, once I get it, I will review the same.


> >
> > Performance Test Result:
> > Run with pgbench for 300 seconds
> >
> > WAL level : hot_standby
> > WAL Size  :   111BF8A8
> > TPS   :   125
> >
> > WAL level : all
> > WAL Size  :   11DB5AF8
> > TPS   :   122
> >
> > * TPS is almost constant but WAL size is increased around 11M.
> >
> > This is the first level of observation, I will continue to test few
> more scenarios including performance test on standby.
> 
> Thank you for performance testing.
> According of test result, TPS of 'all'  lower than 'hot_standby',but
> WAL size is increased.
> I think that it should be separate parameter.
> And TPS on master side is is almost constant. so this patch might have
> several benefit for performance improvement on standby side if the
> result of performance test on standby side is improved.

[Performance test on standby:]

I have executed pgbench on master with WAL LEVEL "hot_stanby" and "all" option, 
and after that run pgbench on standby with "select-only" option.

WAL LEVEL (on master): hot_standby  
Select TPS (on standby)  : 4098

WAL LEVEL (on master): all  
Select TPS (on standby)  : 4115


Regards,
Dilip








-- 
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] [PoC] pgstattuple2: block sampling to reduce physical read

[firoz e v]

On 16/09/13 16:20, Satoshi Nagayasu wrote:
> Thanks for checking. Fixed to eliminate SnapshotNow.

Looking forward to get a new patch, incorporating the comments, that are 
already given in the following mails:

1. Jaime Casanova: "The name pgstattuple2, doesn't convince me... maybe you can 
use pgstattuple() if you use a second argument (percentage of the sample) to 
overload the function".
(http://www.postgresql.org/message-id/5265ad16.3090...@catalyst.net.nz)

The comment related to having an argument, to mention the sampling number, is 
also given by Greg Smith: "There should be an input parameter to the function 
for how much sampling to do"
(http://www.postgresql.org/message-id/51ee62d4.7020...@2ndquadrant.com)

2. Yourself: "I think it could be improved by sorting sample block numbers 
before physical block reads in order to eliminate random access on the disk."
(http://www.postgresql.org/message-id/525779c5.2020...@uptime.jp) for which, 
Mark Kirkwood , has given a rough patch.

Regards,
Firoz EV

Re: [HACKERS] Logging WAL when updating hintbit

[Dilip kumar]

On 19 November 2013 22:19, Sawada Masahiko Wrote

> >>
> >> Thank you for comment.
> >> Actually, I had thought to add separate parameter.
> >
> > I think that he said that if you can proof that amount of WAL is
> > almost same and without less performance same as before, you might
> not
> > need to separate parameter in your patch.
> >
> 
> Thanks!
> I took it wrong.
> I think that there are quite a few difference amount of WAL.
> 
> > Did you test about amount of WAL size in your patch?
> 
> Not yet. I will do that.

1. Patch applies cleanly to master HEAD.
2. No Compilation Warning.
3. It works as per the patch expectation.

Some Suggestion:
1. Add new WAL level ("all") in comment in postgresql.conf 
   wal_level = hot_standby # minimal, archive, or 
hot_standby


Performance Test Result:
Run with pgbench for 300 seconds

WAL level : hot_standby
WAL Size  :   111BF8A8
TPS   :   125

WAL level : all
WAL Size  :   11DB5AF8
TPS   :   122 

* TPS is almost constant but WAL size is increased around 11M.

This is the first level of observation, I will continue to test few more 
scenarios including performance test on standby.

Regards,
Dilip Kumar










-- 
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] [GENERAL] pg_upgrade ?deficiency

[Bruce Momjian]

Sending to hackers for comment;   seems setting
default_transaction_read_only to true via ALTER database/user or
postgresql.conf can cause pg_dump, pg_dumpall, and pg_upgrade failures. 
We are considering the right solution:

---

On Fri, Nov 22, 2013 at 01:32:30PM -0500, Bruce Momjian wrote:
> On Thu, Nov 21, 2013 at 06:22:50AM -0800, Kevin Grittner wrote:
> > > Well, pg_upgrade can't handle every possible configuration.  How
> > > do we even restore into such a database?  You marked the database
> > > as read-only, and pg_upgrade is going to honor that and not
> > > modify it.
> > 
> > That interpretation makes no sense to me.  I know of users who have
> > databases where 90% of their transactions don't modify data, so
> > they set the *default* for transactions to read only, and override
> > that for transactions that are read write.  The default is not, and
> > never has been, a restriction on what is allowed -- it is a default
> > that is quite easy to override.  If we have tools that don't handle
> > that correctly, I consider that a bug.
> 
> OK, this is good information to hear.
> 
> > > I believe a pg_dumpall restore might fail in the same way.
> > 
> > Then it should also be fixed.
> 
> Yes, that is easy to do.
> 
> > > You need to change the default on the old cluster before
> > > upgrading.  It is overly cumbersome to set the
> > > default_transaction_read_only for every database connection
> > 
> > Why is this any different from other settings we cover at the front
> > of pg_dump output?:
> > 
> > | SET statement_timeout = 0;
> > | SET lock_timeout = 0;
> > | SET client_encoding = 'UTF8';
> > | SET standard_conforming_strings = on;
> > | SET check_function_bodies = false;
> > | SET client_min_messages = warning;
> > 
> > > and there are many other settings that might also cause failures.
> > 
> > You mean, like the above?
> > 
> > > What you might be able to do is to set PGOPTIONS to "-c
> > > default_transaction_read_only=false" and run pg_upgrade.  If more
> > > people report this problem, I could document this work-around.
> > 
> > This is most likely to bite those using serializable transactions
> > for data integrity, because declaring transactions read only makes
> > a huge difference in performance in those cases.  That is where I
> > have seen people set the default for read only to on; they want to
> > explicitly set it off only when needed.
> > 
> > I would be happy to supply a patch to treat
> > default_transaction_read_only the same as statement_timeout or
> > standard_conforming_strings in pg_dump and related utilities. 
> > Since it causes backup/restore failure on perfectly valid databases
> > I even think this is a bug which merits back-patching.
> 
> Not sure about backpatching.  default_transaction_read_only has been
> around since 7.4.  Setting it to true would cause pg_dump to fail unless
> you changed the database setting, and pg_dumpall would fail completely
> as there is no way to turn off the database setting.
> 
> The problem is that I don't remember any report of this failing in
> pg_dump, pg_dumpall, or pg_upgrade, so saying it is a major issue is
> hard to accept.
> 
> However, looking forward, I think we should add it to pg_dump (and hence
> pg_dumpall), and we should fix pg_upgrade so it is more reliable.  I am
> thinking we should either document in pg_upgrade the use of PGOPTIONS to
> avoid this issue, or have pg_upgrade append to PGOPTIONS in its
> environment to use some of pg_dump's resets, and that will be passed to
> libpq connections, psql, and all the utilities pg_upgrade calls.

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

  + Everyone has their own god. +


-- 
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] COPY table FROM STDIN doesn't show count tag

[Rajeev rastogi]

On 20 November, Amit Khandekar wrote:
I hope you meant to write test case as psql -d postgres -c "\copy tab from 
stdin; insert into tab values ('lll', 3)", as if we are reading from file, 
then the above issue does not come.
>>>I meant COPY with a slash. \COPY is equivalent to COPY FROM STDIN. So the 
>>>issue can also be reproduced by :
>>>\COPY tab from 'client_filename' ...

 I have modified the patch as per your comment and same is attached with 
 this mail.

>>>Thanks. The COPY FROM looks good.
>>OK..Thanks
>>>With the patch applied, \COPY TO 'data_file' command outputs the  COPY 
>>>status into the data file, instead of printing it in the psql session.
>>>postgres=# \copy tab to '/tmp/fout';
>>>postgres=#
 >>>$ cat /tmp/fout
>>>ee  909
>>>COPY 1
>>>This is probably because client-side COPY overrides the pset.queryFout with 
>>>its own destination file, and while printing the COPY status, the overridden 
>>>file pointer is not yet reverted back.
 >>This looks to be an issue without our new patch also. Like I tried following 
 >>command and output was as follows:
>>rajeev@linux-ltr9:~/9.4gitcode/install/bin>
>> ./psql -d postgres -c "\copy tbl to 'new.txt';insert into tbl values(55);"
>>rajeev@linux-ltr9:~/9.4gitcode/install/bin>
>> cat new.txt
>>5
>>67
>>5
>>67
>>2
>>2
>>99
>>1
>>1
>INSERT 0 1

>Ok. Yes it is an existing issue. Because we are now printing the COPY status 
>even for COPY TO, the existing issue surfaces too easily with the patch. \COPY 
>TO is a pretty common scenario. And it does not have to have a subsequent 
>another command
>to reproduce the issue Just a single \COPY TO command reproduces the issue.
>>I have fixed the same as per your suggestion by resetting the pset.queryFout 
>>after the function call "handleCopyOut".
>>!  pset.queryFout = stdout;

>The original pset.queryFout may not be stdout. psql -o option can override the 
>stdout default.

>I think solving the \COPY TO part is going to be a  different (and an 
>involved) issue to solve than the COPY FROM. Even if we manage to revert back 
>the queryFout, I think ProcessResult() is not the right place to do it. 
>ProcessResult() should not
> assume that  somebody else has changed queryFout. Whoever has changed it 
> should revert it. Currently, do_copy() is indeed doing this correctly:

>  save_file = *override_file;
>  *override_file = copystream;
>  success = SendQuery(query.data);
>  *override_file = save_file;

>But the way SendQuery() itself processes the results and prints them, is 
>conflicting with the above.

>So I think it is best to solve this as a different issue, and we should , for 
>this commitfest,  fix only COPY FROM. Once the \COPY existing issue is solved, 
>only then we can start printing the \COPY TO status as well.

You mean to say that I should change the patch to keep only COPY FROM related 
changes and remove changes related to COPY TO.
If yes, then I shall change the patch accordingly  and also mention same in 
documentation also.
Please let me know about this so that I can share the modified patch.

Thanks and Regards,
Kumar Rajeev Rastogi

Re: [HACKERS] Add min and max execute statement time in pg_stat_statement

[Rajeev rastogi]

On 14 November 2013, Kondo Mitsumasa wrote:

> Subject: Re: [HACKERS] Add min and max execute statement time in
> pg_stat_statement
> 
> Oh! Sorry...
> I forgot to attach my latest patch.

* Is the patch in a patch format which has context?
No

* Does it apply cleanly to the current git master?
Yes. 

* Does it compiles without any warning?
No. Compilation fails on windows platform. 
.\contrib\pg_stat_statements\pg_stat_statements.c(1232) : error 
C2102: '&' requires l-value
1232 Line is: values[i++] = Float8GetDatumFast(sqrt(sqtime - avtime * 
avtime));

Thanks and Regards,
Kumar Rajeev Rastogi


-- 
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] PostgreSQL Service on Windows does not start. ~ "is not a valid Win32 application"

[Rajeev rastogi]

ON 11 November 2013, Naoya Anzai Wrote:

>> 
>> Hi Amit,
>> 
> > I have uploaded your patch for next commit fest, hope you can support
> > it if there is any feedback for your patch by reviewer/committer.
> Thanks! Okay, I will support you.

1. Patch applies cleanly to master HEAD.
2. No Compilation Warning.
3. It works as per the patch expectation.

One suggestion:
Instead of using sizeof(cmdLine),
a. Can't we use strlen  (hence small 'for' loop).
b. Or use memmove to move one byte. 

Thanks and Regards,
Kumar Rajeev Rastogi


-- 
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] COPY table FROM STDIN doesn't show count tag

[Rajeev rastogi]

On 19 November 2013, Amit Khandekar wrote:
>On 18 November 2013 18:00, Rajeev rastogi 
>mailto:rajeev.rast...@huawei.com>> wrote:
>>On 18 November 2013, Amit Khandekar wrote:
> >>Please find the patch for the same and let me know your suggestions.
>>>In this call :
> >> success = handleCopyIn(pset.db, 
> >> pset.cur_cmd_source,
> >>
> >>PQbinaryTuples(*results), &intres) && success;
> >> if (success && intres)
> >> success = 
> >> PrintQueryResults(intres);
>>>Instead of handling of the result status this way, what if we use the 
>>>ProcessResult()  argument 'result' to pass back the COPY result status to 
>>>the caller ? We already call PrintQueryResults(results) after the 
>>>ProcessResult() call. So we don't have to  have a COPY-specific 
>>>PrintQueryResults() call. Also, if there is a subsequent SQL command in the 
>>>same query string, the consequence of the patch is that the client prints 
>>>both COPY output and the last command output. So my suggestion would also 
>>>allow us to be consistent with the general behaviour that only the last SQL 
>>>command output is printed in case of multiple SQL commands. Here is how it 
>>>gets printed with your patch :
>> Thank you for valuable comments. Your suggestion is absolutely correct.
 >>>psql -d postgres -c "\copy tab from '/tmp/st.sql' delimiter ' '; insert 
 >>>into tab values ('lll', 3)"
>>>COPY 1
>>>INSERT 0 1
>>>This is not harmful, but just a matter of consistency.
>>I hope you meant to write test case as psql -d postgres -c "\copy tab from 
>>stdin; insert into tab values ('lll', 3)", as if we are reading from file, 
>>then the above issue does not come.
>I meant COPY with a slash. \COPY is equivalent to COPY FROM STDIN. So the 
>issue can also be reproduced by :
>\COPY tab from 'client_filename' ...

 >>I have modified the patch as per your comment and same is attached with this 
 >>mail.

>Thanks. The COPY FROM looks good.
OK..Thanks

>With the patch applied, \COPY TO 'data_file' command outputs the  COPY status 
>into the data file, instead of printing it in the psql session.

>postgres=# \copy tab to '/tmp/fout';
>postgres=#

>$ cat /tmp/fout
>ee  909
>COPY 1
>This is probably because client-side COPY overrides the pset.queryFout with 
>its own destination file, and while printing the COPY status, the overridden 
>file pointer is not yet reverted back.

This looks to be an issue without our new patch also. Like I tried following 
command and output was as follows:
rajeev@linux-ltr9:~/9.4gitcode/install/bin> ./psql -d postgres -c "\copy tbl to 
'new.txt';insert into tbl values(55);"
rajeev@linux-ltr9:~/9.4gitcode/install/bin> cat new.txt
5
67
5
67
2
2
99
1
1
INSERT 0 1

I have fixed the same as per your suggestion by resetting the pset.queryFout 
after the function call "handleCopyOut".

Please let me know in-case of any other issues.

Thanks and Regards,
Kumar Rajeev Rastogi


copydefectV3.patch
Description: copydefectV3.patch

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

Re: [HACKERS] Suggestion: Issue warning when calling SET TRANSACTION outside transaction block

[Bruce Momjian]

On Fri, Nov 22, 2013 at 12:17:41PM -0500, Bruce Momjian wrote:
> Good points.  I have modified the attached patch to do as you suggested.

Also, I have read through the thread and summarized the positions of the
posters:

  9.3 WARNING ERROR
  SET noneTom, DavidJ, AndresFRobert, Kevin
  SAVEPOINT   error   Tom, DavidJ, Robert, 
AndresF, Kevin
  LOCK, DECLARE   error   Tom, DavidJ, Robert, 
AndresF, Kevin

Everyone seems to agree that SAVEPOINT, LOCK, and DECLARE should remain
as errors.  Everyone also seems to agree that BEGIN and COMMIT should
remain warnings, and ABORT should be changed from notice to warning.

Our only disagreement seems to be how to handle the SET commands, which
used to report nothing.  Would anyone else like to correct or express an
opinion?  Given the current vote count and backward-compatibility,
warning seems to be the direction we are heading.

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

  + Everyone has their own god. +


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

Re: [HACKERS] UNNEST with multiple args, and TABLE with multiple funcs

[Andrew Gierth]

> "Tom" == Tom Lane  writes:

 Tom> [ I assume you forgot a create type footype here ]

yeah, sorry

 Tom> Well, it's not insane on its face.  The rowtype of f in the
 Tom> first example is necessarily a built-on-the-fly record, but in
 Tom> the second case using the properties of the underlying named
 Tom> composite type is possible, and consistent with what happens in
 Tom> 9.3 and earlier.  (Not that I'm claiming we were or are totally
 Tom> consistent ...)

Right, but your changes to the code make it look like there was an
intended change there - with the scan type tupdesc being forced to
RECORD type and its column names changed.

-- 
Andrew (irc:RhodiumToad)


-- 
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] Building on S390

[Michael Meskes]

On Fri, Nov 22, 2013 at 11:27:45AM -0500, Tom Lane wrote:
> I think this is probably nonsense.  I spent ten years maintaining Postgres
> for Red Hat, and I never saw any such failure on s390 in their packages.
> If -fpic weren't good enough for shared libraries on s390, how'd any of
> those builds get through their regression tests?

You've got a point here.

> It may well be that *mixing* -fpic and -fPIC is a bad idea, but I'd say
> that points to an error in something XC is doing, because the core
> Postgres build doesn't use -fPIC anywhere for Linux/s390, AFAICS.

I actually only compared to the Debian build which *does* have -fPIC and indeed
it seems it adds -fPIC unconditionally. But then the PostgreSQL package works
flawlessly which obviously points to XC for the problem. I give you that.

> Furthermore, if we change that convention now, we're going to increase
> the risk of such mixing failures for other people.

Sure, but if this a bug we should. I'm not saying it is, I simply don't know.

The thread is starting with my email here
http://lists.debian.org/debian-s390/2013/10/msg8.html and the reply said:

It uses -fpic instead of -fPIC.

No, I'm not shortening that email reply here. :)

Checking the Debian logs it appears that all calls use *both* which seems to do
the right thing. And yes, it appears there is a change in XC that makes it
break. But still, I would think there has to be a correct set of options.

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
Jabber: michael.meskes at gmail dot com
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL


-- 
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] Suggestion: Issue warning when calling SET TRANSACTION outside transaction block

[Bruce Momjian]

On Fri, Nov 22, 2013 at 10:24:35AM -0300, Alvaro Herrera wrote:
> Bruce Momjian escribió:
> 
> > OK, here is a patch which changes ABORT from NOTICE to WARNING, and SET
> > from ERROR (which is new in 9.4) to WARNING.
> 
> I don't like that this patch changes RequireTransactionChain() from
> actually requiring one, to a function that maybe requires a transaction
> chain, and maybe it only complains about there not being one.  I mean,
> it's like you had named the new throwError boolean as "notReally" or
> something like that.  Also, the new comment paragraph is bad because it
> explains who must pass true/false, instead of what's the effect of each
> value (and let the callers choose which value to pass).
> 
> I would create a separate function to implement this, maybe
> WarnUnlessInTransactionBlock() or something like that.  That would make
> the patch a good deal smaller (because not changing existing callers).

Good points.  I have modified the attached patch to do as you suggested.

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

  + Everyone has their own god. +
diff --git a/doc/src/sgml/ref/abort.sgml b/doc/src/sgml/ref/abort.sgml
new file mode 100644
index 246e8f8..f3a2fa8
*** a/doc/src/sgml/ref/abort.sgml
--- b/doc/src/sgml/ref/abort.sgml
*** ABORT [ WORK | TRANSACTION ]
*** 63,70 

  

!Issuing ABORT when not inside a transaction does
!no harm, but it will provoke a warning message.

   
  
--- 63,69 

  

!Issuing ABORT outside of a transaction block has no effect.

   
  
diff --git a/doc/src/sgml/ref/rollback.sgml b/doc/src/sgml/ref/rollback.sgml
new file mode 100644
index b265545..4f79621
*** a/doc/src/sgml/ref/rollback.sgml
--- b/doc/src/sgml/ref/rollback.sgml
*** ROLLBACK [ WORK | TRANSACTION ]
*** 59,66 

  

!Issuing ROLLBACK when not inside a transaction does
!no harm, but it will provoke a warning message.

   
  
--- 59,66 

  

!Issuing ROLLBACK outside of a transaction
!block has no effect.

   
  
diff --git a/doc/src/sgml/ref/set.sgml b/doc/src/sgml/ref/set.sgml
new file mode 100644
index 6290c9d..5a84f69
*** a/doc/src/sgml/ref/set.sgml
--- b/doc/src/sgml/ref/set.sgml
*** SET [ SESSION | LOCAL ] TIME ZONE { 
Specifies that the command takes effect for only the current
transaction.  After COMMIT or ROLLBACK,
!   the session-level setting takes effect again.
!   PostgreSQL reports an error if
!   SET LOCAL is used outside a transaction block.
   
  
 
--- 110,117 
   
Specifies that the command takes effect for only the current
transaction.  After COMMIT or ROLLBACK,
!   the session-level setting takes effect again.  This has no effect
!   outside of a transaction block.
   
  
 
diff --git a/doc/src/sgml/ref/set_constraints.sgml b/doc/src/sgml/ref/set_constraints.sgml
new file mode 100644
index 895a5fd..a33190c
*** a/doc/src/sgml/ref/set_constraints.sgml
--- b/doc/src/sgml/ref/set_constraints.sgml
*** SET CONSTRAINTS { ALL | 
 This command only alters the behavior of constraints within the
!current transaction. Thus, if you execute this command outside of a
!transaction block
!(BEGIN/COMMIT pair), it will
!generate an error.

   
  
--- 99,105 
  

 This command only alters the behavior of constraints within the
!current transaction.  This has no effect outside of a transaction block.

   
  
diff --git a/doc/src/sgml/ref/set_transaction.sgml b/doc/src/sgml/ref/set_transaction.sgml
new file mode 100644
index 391464a..e90ff4a
*** a/doc/src/sgml/ref/set_transaction.sgml
--- b/doc/src/sgml/ref/set_transaction.sgml
*** SET SESSION CHARACTERISTICS AS TRANSACTI
*** 185,191 

 If SET TRANSACTION is executed without a prior
 START TRANSACTION or BEGIN,
!it will generate an error.

  

--- 185,191 

 If SET TRANSACTION is executed without a prior
 START TRANSACTION or BEGIN,
!it will have no effect.

  

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
new file mode 100644
index 0591f3f..bab048d
*** a/src/backend/access/transam/xact.c
--- b/src/backend/access/transam/xact.c
*** static void CallSubXactCallbacks(SubXact
*** 265,270 
--- 265,272 
  	 SubTransactionId mySubid,
  	 SubTransactionId parentSubid);
  static void CleanupTransaction(void);
+ static void CheckTransactionChain(bool isTopLevel, bool throwError,
+ 	 const char *stmtType);
  static void CommitTransaction(void);
  static TransactionId RecordTransactionAbort(bool isSubXact);
  static void StartTransaction(void);
*** PreventTransactionChain(bool isTopLevel,
*** 2949,2954 
--- 2951,2976 
  }
  
  /*
+  *	These two functions allow for warnings or er

Re: [HACKERS] UNNEST with multiple args, and TABLE with multiple funcs

[Tom Lane]

Andrew Gierth  writes:
> Is this intended:

[ I assume you forgot a create type footype here ]

> create function foo() returns setof footype language plpgsql
>   as $f$ begin return next row(1,true); end; $f$;

> select pg_typeof(f), row_to_json(f) from foo() with ordinality f(p,q);
>  pg_typeof |   row_to_json   
> ---+-
>  record| {"p":1,"q":true,"ordinality":1}
> (1 row)

> select pg_typeof(f), row_to_json(f) from foo() f(p,q);
>  pg_typeof |   row_to_json
> ---+--
>  footype   | {"a":1,"b":true}
> (1 row)

Well, it's not insane on its face.  The rowtype of f in the first example
is necessarily a built-on-the-fly record, but in the second case using
the properties of the underlying named composite type is possible, and
consistent with what happens in 9.3 and earlier.  (Not that I'm claiming
we were or are totally consistent ...)

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] Can we trust fsync?

[Claudio Freire]

On Fri, Nov 22, 2013 at 1:16 PM, Tom Lane  wrote:
>> The original mail was referencing a problem with syncing *meta* data
>> though. The semantics around meta data syncs are much less clearly
>> specified, in part because file systems traditionally made nearly all meta
>> data operations synchronous. Doing plug-pull testing on Postgres would not
>> test meta data syncing very well since Postgres specifically avoids doing
>> much meta data operations by overwriting existing files and blocks as much
>> as possible.
>
> True.  You're better off with a specialized testing program.  (Though
> now you mention it, I wonder whether that program was stressing metadata
> or not.)


You can always stress metadata by leaving atime updates in their full
setting (whatever it is for that filesystem).


-- 
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] Building on S390

[Tom Lane]

Michael Meskes  writes:
> I spend some time trying to figure out why PostgreSQL builds on
> S390-Linux, but Postgres-XC doesn't. Well at least this holds for the Debian
> packages. So far I haven't figured it out.  However, it appears to me that the
> build should fail for both. I'm not an S390 expert by any means, but I was 
> told
> that S390 requires -fPIC and the build failure in the Debian package of XC 
> came
> from a stray -fpic that was used together with -fPIC. But alas the PostgreSQL
> build has both as well.

I think this is probably nonsense.  I spent ten years maintaining Postgres
for Red Hat, and I never saw any such failure on s390 in their packages.
If -fpic weren't good enough for shared libraries on s390, how'd any of
those builds get through their regression tests?

It may well be that *mixing* -fpic and -fPIC is a bad idea, but I'd say
that points to an error in something XC is doing, because the core
Postgres build doesn't use -fPIC anywhere for Linux/s390, AFAICS.
Furthermore, if we change that convention now, we're going to increase
the risk of such mixing failures for other people.

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] Can we trust fsync?

[Tom Lane]

Greg Stark  writes:
> On Thu, Nov 21, 2013 at 1:43 AM, Tom Lane  wrote:
>> Also, it's not that hard to do plug-pull testing to verify that your
>> system is telling the truth about fsync.  This really ought to be part
>> of acceptance testing for any new DB server.

> I've never tried it but I always wondered how easy it was to do. How would
> you ever know you had tested it enough?

I used the program Greg Smith recommends on our wiki (can't remember the
name offhand) when I got a new house server this spring.  With the RAID
card configured for writethrough and no battery, it failed all over the
place.  Fixed those configuration bugs, it was okay three or four times
in a row, which was good enough for me.

> The original mail was referencing a problem with syncing *meta* data
> though. The semantics around meta data syncs are much less clearly
> specified, in part because file systems traditionally made nearly all meta
> data operations synchronous. Doing plug-pull testing on Postgres would not
> test meta data syncing very well since Postgres specifically avoids doing
> much meta data operations by overwriting existing files and blocks as much
> as possible.

True.  You're better off with a specialized testing program.  (Though
now you mention it, I wonder whether that program was stressing metadata
or not.)

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] Can we trust fsync?

[Greg Stark]

On Thu, Nov 21, 2013 at 1:43 AM, Tom Lane  wrote:

> Also, it's not that hard to do plug-pull testing to verify that your
> system is telling the truth about fsync.  This really ought to be part
> of acceptance testing for any new DB server.
>

I've never tried it but I always wondered how easy it was to do. How would
you ever know you had tested it enough?


The original mail was referencing a problem with syncing *meta* data
though. The semantics around meta data syncs are much less clearly
specified, in part because file systems traditionally made nearly all meta
data operations synchronous. Doing plug-pull testing on Postgres would not
test meta data syncing very well since Postgres specifically avoids doing
much meta data operations by overwriting existing files and blocks as much
as possible. You would have to test doing table extensions or pulling the
plug immediately after switching xlog files repeatedly to have any coverage
at all there.


-- 
greg

[HACKERS] Building on S390

[Michael Meskes]

Hi,

I spend some time trying to figure out why PostgreSQL builds on
S390-Linux, but Postgres-XC doesn't. Well at least this holds for the Debian
packages. So far I haven't figured it out.  However, it appears to me that the
build should fail for both. I'm not an S390 expert by any means, but I was told
that S390 requires -fPIC and the build failure in the Debian package of XC came
from a stray -fpic that was used together with -fPIC. But alas the PostgreSQL
build has both as well.

Anyway, I changed src/makefiles/Makefile.linux to include

ifeq "$(findstring s390,$(host_cpu))" "s390"
CFLAGS_SL = -fPIC
else

before setting CFLAGS_SL = -fpic et voila XC builds just fine on S390. 

So I wonder shouldn't we use -fPIC instead of -fpic for S390 in general?

Michael

-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
Jabber: michael.meskes at gmail dot com
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL


-- 
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] MultiXact pessmization in 9.3

[Andres Freund]

On 2013-11-22 12:04:31 -0300, Alvaro Herrera wrote:
> Andres Freund wrote:
> 
> > While looking at the multixact truncation code (mail nearby), I've
> > noticed that there's a significant difference in the way multixact
> > members are accessed since fkey locks were introduced:
> > 
> > <9.3 when heap_lock_tuple finds a XMAX_IS_MULTI tuple it executes
> > MultiXactIdWait() which in turn uses GetMultiXactIdMembers() to get the
> > xids to wait for. But it skips the lookup if the mxid we lookup is older
> > than OldestVisibleMXactId.
> > 
> > 9.3+ instead always looks up the members because GetMultiXactIdMembers()
> > is also used in cases where we need to access old xids (to check whether
> > members have commited in non-key updates).
> 
> But HeapTupleSatisfiesUpdate(), called at the top of heap_lock_tuple,
> has already called MultiXactIdIsRunning() (which calls GetMembers)
> before that's even considered.  So the call heap_lock_tuple should have
> members obtained from the multixact.c cache.
> 
> Am I misunderstanding which code path you mean?

Yes, somewhat: <9.3 GetMultiXactIdMembers() didn't do any work if the
multixact was old enough:
if (MultiXactIdPrecedes(multi, OldestVisibleMXactId[MyBackendId]))
{
debug_elog2(DEBUG2, "GetMembers: it's too old");
*xids = NULL;
return -1;
}
so, in OLTP style workloads, doing a heap_lock_tuple() often didn't have
to perform any IO when locking a tuple that previously had been locked
using a multixact. We knew that none of the members could still be
running and thus didn't have to ask the SLRU for them since a
not-running member cannot still have a row locked.

Now, fkey locks changed that because for !HEAP_XMAX_LOCK_ONLY mxacts, we
need to look up the members to get the update xid and check whether it
has committed or aborted, even if we know that it isn't currently
running anymore due do OldestVisibleMXactId. But there's absolutely no
need to do that for HEAP_XMAX_LOCK_ONLY mxacts, the old reasoning for
not looking up the members if old is just fine.

Additionally, we don't ever set hint bits indicating that a
HEAP_XMAX_IS_MULTI & !HEAP_XMAX_LOCK_ONLY mxact has commited, so we'll
do HeapTupleGetUpdateXid(), TransactionIdIsCurrentTransactionId(),
TransactionIdIsInProgress(), TransactionIdDidCommit() calls for every
HeapTupleSatisfiesMVCC().

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] Minor patch for the uuid-ossp extension

[Andrew Dunstan]

On 11/22/2013 10:19 AM, Alvaro Herrera wrote:

Tom Lane wrote:

roadrunn...@gmx.at writes:
regression=# \echo Use '''CREATE EXTENSION "uuid-ossp"''' to load this file.
Use 'CREATE EXTENSION "uuid-ossp"' to load this file.

Does that look reasonable to people?

+1



+1

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] Minor patch for the uuid-ossp extension

[Alvaro Herrera]

Tom Lane wrote:
> roadrunn...@gmx.at writes:

> regression=# \echo Use '''CREATE EXTENSION "uuid-ossp"''' to load this file. 
> Use 'CREATE EXTENSION "uuid-ossp"' to load this file.
> 
> Does that look reasonable to people?

+1

-- 
Á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] Minor patch for the uuid-ossp extension

[Tom Lane]

roadrunn...@gmx.at writes:
> When trying to add the extension with \i it writes an error message:
>  Use "CREATE EXTENSION uuid-ossp" to load this file.

> Unfortunatly this does not work for extensions with dashes. Must CREATE 
> EXTENSION "uuid-ossp". Proposed patch is attached.

[ memo to self: never, ever accept another contrib module whose name
isn't a plain SQL identifier ]

Yeah, that's a problem, but I don't find your solution acceptable:

-\echo Use "CREATE EXTENSION uuid-ossp" to load this file. \quit
+\echo Use CREATE EXTENSION "uuid-ossp" to load this file. \quit

That's just ignoring the English text quoting convention that these
messages are trying to follow.  I guess we could shade the convention a
bit by using single not double quotes around the recommended command.
psql doesn't make that tremendously easy, but a bit of experimentation
says this works:

regression=# \echo Use '''CREATE EXTENSION "uuid-ossp"''' to load this file. 
Use 'CREATE EXTENSION "uuid-ossp"' to load this file.

Does that look reasonable to people?

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] new unicode table border styles for psql

[Merlin Moncure]

On Fri, Nov 22, 2013 at 8:45 AM, Alvaro Herrera
 wrote:
> Pavel Stehule escribió:
>
>> 2013/11/21 Peter Eisentraut 
>
>> > Maybe make the border setting a string containing the various characters
>> > by index.  Then everyone can create their own crazy borders.
>> >
>> I seriously though about it, but not sure if it is good way.
>
> How about having a single "unicode" line style, and then have a
> different \pset setting to determine exactly what chars to print?  This
> wouldn't allow for programmability, but it seems better UI to me.
> This proliferation of unicode line style names seems odd.

That makes sense to me, especially if you could pass escapes.

merlin


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

Re: [HACKERS] MultiXact pessmization in 9.3

[Alvaro Herrera]

Andres Freund wrote:

> While looking at the multixact truncation code (mail nearby), I've
> noticed that there's a significant difference in the way multixact
> members are accessed since fkey locks were introduced:
> 
> <9.3 when heap_lock_tuple finds a XMAX_IS_MULTI tuple it executes
> MultiXactIdWait() which in turn uses GetMultiXactIdMembers() to get the
> xids to wait for. But it skips the lookup if the mxid we lookup is older
> than OldestVisibleMXactId.
> 
> 9.3+ instead always looks up the members because GetMultiXactIdMembers()
> is also used in cases where we need to access old xids (to check whether
> members have commited in non-key updates).

But HeapTupleSatisfiesUpdate(), called at the top of heap_lock_tuple,
has already called MultiXactIdIsRunning() (which calls GetMembers)
before that's even considered.  So the call heap_lock_tuple should have
members obtained from the multixact.c cache.

Am I misunderstanding which code path you mean?

-- 
Á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] commit fest 2013-11 week 1 report

[Euler Taveira]

On 22-11-2013 11:07, Pavel Golub wrote:
> Is is possible to add small patch to the current commit fest?
> 
No. Deadline was 11/15. Add it to next CF [1].


[1] https://commitfest.postgresql.org/action/commitfest_view?id=21


-- 
   Euler Taveira   Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento


-- 
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] new unicode table border styles for psql

[Alvaro Herrera]

Pavel Stehule escribió:

> 2013/11/21 Peter Eisentraut 

> > Maybe make the border setting a string containing the various characters
> > by index.  Then everyone can create their own crazy borders.
> >
> I seriously though about it, but not sure if it is good way.

How about having a single "unicode" line style, and then have a
different \pset setting to determine exactly what chars to print?  This
wouldn't allow for programmability, but it seems better UI to me.
This proliferation of unicode line style names seems odd.

-- 
Á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] address sanitizer crash

[Tom Lane]

Peter Eisentraut  writes:
> AddressSanitizer (http://clang.llvm.org/docs/AddressSanitizer.html)
> (also available through gcc, but I used clang), reports one bug, which
> I traced down to this code in ruleutils.c:

> [elsewhere:]
> #define ViewSelectRuleName "_RETURN"

> /*
>  * Get the pg_rewrite tuple for the view's SELECT rule
>  */
> args[0] = ObjectIdGetDatum(viewoid);
> args[1] = PointerGetDatum(ViewSelectRuleName);
> nulls[0] = ' ';
> nulls[1] = ' ';
> spirc = SPI_execute_plan(plan_getviewrule, args, nulls, true, 2);

Yes, the plan clearly is built expecting $2 to be of type "name",
so this has been wrong since day 1.  Amazing that no actual bug reports
have surfaced from it.

> I think the correct code is something like
> args[1] = DirectFunctionCall1(namein, 
> CStringGetDatum(ViewSelectRuleName));

That would be OK.  The more usual coding pattern is to declare a local
NameData variable, namestrcpy into that, and then PointerGetDatum on
the variable's address is actually correct.  However, that's just
micro-optimization that I don't think we care about here.

> [I also think that the 2 here is wrong, probably thinking it means 2
> arguments, but it means 2 result rows, but only one is needed.  But
> that's unrelated to the bug.]

Yes, while harmless that's clearly in error, should be zero.  The other
call of SPI_execute_plan in ruleutils.c has the same thinko.

Please fix and back-patch.

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] new unicode table border styles for psql

[Pavel Stehule]

2013/11/22 Merlin Moncure 

> On Fri, Nov 22, 2013 at 2:23 AM, Pavel Stehule 
> wrote:
> > Hello
> >
> >
> > 2013/11/21 Merlin Moncure 
> >>
> >> On Thu, Nov 21, 2013 at 1:09 AM, Pavel Stehule  >
> >> wrote:
> >> > Hello
> >> >
> >> > I wrote new styles for  psql table borders.
> >> >
> >> > http://postgres.cz/wiki/Pretty_borders_in_psql
> >> >
> >> > This patch is simply and I am think so some styles can be interesting
> >> > for
> >> > final presentation.
> >> >
> >> great. hm, maybe we could integrate color? (see:
> >>
> >>
> http://merlinmoncure.blogspot.com/2012/09/psql-now-with-splash-of-color.html
> ).
> >
> >
> > it is next possible enhancing - I would to go forward in small steps,
> please
> > :)
> >
> > minimally (and independent on proposed patch) we can introduce some like
> > "final regexp filtering"  - that can be used for this or other purposes.
>
> Yeah.  A per field regexp would do the trick.  As you have it, I like
> Peter's idea best.  Being able to specify the various character codes
> makes a lot of sense.
>

there is other issue - simply parser will be really user unfriendly, and
user friendly parser will not by simply :(

have you some idea about input format?

Regards

Pavel


>
> merlin
>

Re: [HACKERS] new unicode table border styles for psql

[Merlin Moncure]

On Fri, Nov 22, 2013 at 2:23 AM, Pavel Stehule  wrote:
> Hello
>
>
> 2013/11/21 Merlin Moncure 
>>
>> On Thu, Nov 21, 2013 at 1:09 AM, Pavel Stehule 
>> wrote:
>> > Hello
>> >
>> > I wrote new styles for  psql table borders.
>> >
>> > http://postgres.cz/wiki/Pretty_borders_in_psql
>> >
>> > This patch is simply and I am think so some styles can be interesting
>> > for
>> > final presentation.
>> >
>> great. hm, maybe we could integrate color? (see:
>>
>> http://merlinmoncure.blogspot.com/2012/09/psql-now-with-splash-of-color.html).
>
>
> it is next possible enhancing - I would to go forward in small steps, please
> :)
>
> minimally (and independent on proposed patch) we can introduce some like
> "final regexp filtering"  - that can be used for this or other purposes.

Yeah.  A per field regexp would do the trick.  As you have it, I like
Peter's idea best.  Being able to specify the various character codes
makes a lot of sense.

merlin


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

Re: [HACKERS] Status of FDW pushdowns

[Merlin Moncure]

On Thu, Nov 21, 2013 at 6:43 PM, Shigeru Hanada
 wrote:
> 2013/11/22 Tom Lane :
>> Merlin Moncure  writes:
>>> On Thu, Nov 21, 2013 at 9:05 AM, Bruce Momjian  wrote:
 I know join pushdowns seem insignificant, but it helps to restrict what
 data must be passed back because you would only pass back joined rows.
>>
>>> By 'insignificant' you mean 'necessary to do any non-trivial real
>>> work'.   Personally, I'd prefer it if FDW was extended to allow
>>> arbitrary parameterized queries like every other database connectivity
>>> API ever made ever.
>>
>> [ shrug... ]  So use dblink.  For better or worse, the FDW stuff is
>> following the SQL standard's SQL/MED design, which does not do it
>> like that.
>
> Pass-through mode mentioned in SQL/MED standard might be what he wants.

happen to have a link handy?

merlin


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

Re: Custom Scan APIs (Re: [HACKERS] Custom Plan node)

[Jim Mlodgenski]

KaiGai


On Tue, Nov 19, 2013 at 9:41 AM, Kohei KaiGai  wrote:

> Thanks for your review.
>
> 2013/11/19 Jim Mlodgenski :
> > My initial review on this feature:
> > - The patches apply and build, but it produces a warning:
> > ctidscan.c: In function ‘CTidInitCustomScanPlan’:
> > ctidscan.c:362:9: warning: unused variable ‘scan_relid’
> [-Wunused-variable]
> >
> This variable was only used in Assert() macro, so it causes a warning if
> you
> don't put --enable-cassert on the configure script.
> Anyway, I adjusted the code to check relid of RelOptInfo directly.


>
The warning is now gone.


> > I'd recommend that you split the part1 patch containing the ctidscan
> contrib
> > into its own patch. It is more than half of the patch and its certainly
> > stands on its own. IMO, I think ctidscan fits a very specific use case
> and
> > would be better off being an extension instead of in contrib.
> >
> OK, I split them off. The part-1 is custom-scan API itself, the part-2 is
> ctidscan portion, and the part-3 is remote join on postgres_fdw.
>

Attached is a patch for the documentation. I think the documentation still
needs a little more work, but it is pretty close. I can add some more
detail to it once finish adapting the hadoop_fdw to using the custom scan
api and have a better understanding of all of the calls.


> Thanks,
> --
> KaiGai Kohei 
>
*** a/doc/src/sgml/custom-scan.sgml	2013-11-18 17:50:02.652039003 -0500
--- b/doc/src/sgml/custom-scan.sgml	2013-11-22 09:09:13.624254649 -0500
***
*** 8,47 
handler for
   
   
!   Custom-scan API enables extension to provide alternative ways to scan or
!   join relations, being fully integrated with cost based optimizer,
!   in addition to the built-in implementation.
!   It consists of a set of callbacks, with a unique name, to be invoked during
!   query planning and execution. Custom-scan provider should implement these
!   callback functions according to the expectation of API.
   
   
!   Overall, here is four major jobs that custom-scan provider should implement.
!   The first one is registration of custom-scan provider itself. Usually, it
!   shall be done once at _PG_init() entrypoint on module
!   loading.
!   The other three jobs shall be done for each query planning and execution.
!   The second one is submission of candidate paths to scan or join relations,
!   with an adequate cost, for the core planner.
!   Then, planner shall chooses a cheapest path from all the candidates.
!   If custom path survived, the planner kicks the third job; construction of
!   CustomScan plan node, being located within query plan
!   tree instead of the built-in plan node.
!   The last one is execution of its implementation in answer to invocations
!   by the core executor.
   
   
!   Some of contrib module utilize the custom-scan API. It may be able to
!   provide a good example for new development.

 
  
  
   
!   Its logic enables to skip earlier pages or terminate scan prior to
!   end of the relation, if inequality operator on ctid
!   system column can narrow down the scope to be scanned, instead of
!   the sequential scan that reads a relation from the head to the end.
   
  
 
--- 8,46 
handler for
   
   
!   The custom-scan API enables an extension to provide alternative ways to scan
!   or join relations leveraging the cost based optimizer. The API consists of a
!   set of callbacks, with a unique names, to be invoked during query planning 
!   and execution. A custom-scan provider should implement these callback 
!   functions according to the expectation of the API.
   
   
!   Overall, there are four major tasks that a custom-scan provider should 
!   implement. The first task is the registration of custom-scan provider itself.
!   Usually, this needs to be done once at the _PG_init() 
!   entrypoint when the module is loading. The remaing three tasks are all done
!   when a query is planning and executing. The second task is the submission of
!   candidate paths to either scan or join relations with an adequate cost for
!   the core planner. Then, the planner will choose the cheapest path from all of
!   the candidates. If the custom path survived, the planner starts the third 
!   task; construction of a CustomScan plan node, located
!   within the query plan tree instead of the built-in plan node. The last task
!   is the execution of its implementation in answer to invocations by the core
!   executor.
   
   
!   Some of contrib modules utilize the custom-scan API. They may provide a good
!   example for new development.

 
  
  
   
!   This custom scan in this module enables a scan to skip earlier pages or
!   terminate prior to end of the relation, if the inequality operator on the
!   ctid system column can narrow down the scope to be
!   scanned, instead of a sequential scan which reads a relation from the
!   head to the end.
   

Re: [HACKERS] commit fest 2013-11 week 1 report

[Pavel Golub]

Hello, Peter.

Is is possible to add small patch to the current commit fest?

You wrote:

PE> We started with

PE> Fri Nov 15

PE> Status Summary. Needs Review: 79, Waiting on Author: 7, Ready for
PE> Committer: 5, Committed: 7, Returned with Feedback: 3, Rejected: 1. Total: 
102.

PE> We are now at

PE> Fri Nov 22

PE> Status Summary. Needs Review: 47, Waiting on Author: 28, Ready
PE> for Committer: 10, Committed: 18, Returned with Feedback: 3, Rejected: 3. 
Total: 109.

PE> (some late arrivals, some patches split)

PE> Progress has been quite good.

PE> Almost all patch authors responded to my call to sign up for reviewing
PE> someone else's patch.

PE> 20 patches are still without reviewer.  Most of those are the typical
PE> difficult topics (indexes, replication), so now might be a good time
PE> for experts in those areas to start picking up the remaining patches.

PE> In the coming week, we will be following up with reviewers to send in
PE> their first review if they haven't already done so.





-- 
With best wishes,
 Pavel  mailto:pa...@gf.microolap.com



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

[HACKERS] address sanitizer crash

[Peter Eisentraut]

AddressSanitizer (http://clang.llvm.org/docs/AddressSanitizer.html)
(also available through gcc, but I used clang), reports one bug, which
I traced down to this code in ruleutils.c:

[elsewhere:]
#define ViewSelectRuleName "_RETURN"


/*
 * Get the pg_rewrite tuple for the view's SELECT rule
 */
args[0] = ObjectIdGetDatum(viewoid);
args[1] = PointerGetDatum(ViewSelectRuleName);
nulls[0] = ' ';
nulls[1] = ' ';
spirc = SPI_execute_plan(plan_getviewrule, args, nulls, true, 2);

[I also think that the 2 here is wrong, probably thinking it means 2
arguments, but it means 2 result rows, but only one is needed.  But
that's unrelated to the bug.]

The datums end up in datumCopy(), which tries to copy 64 bytes of
"name" data, overrunning the end of the string.

I think the correct code is something like

args[1] = DirectFunctionCall1(namein, CStringGetDatum(ViewSelectRuleName));

which indeed makes the crash go away.  (A more explicit string-to-name
function might be useful and could also be used in other places.)

This is the only remaining issue found by AddressSanitizer that is
clearly attributable to the PostgreSQL source code (after the recently
fixed issue with memcpy with identical arguments).  There are crashes
in PL/Perl and PL/Python, but it's not clear to me yet whose fault
they are.


-- 
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] Data corruption issues using streaming replication on 9.0.14/9.2.5/9.3.1

[Andres Freund]

On 2013-11-22 15:01:10 +0200, Heikki Linnakangas wrote:
> On 21.11.2013 22:55, Andres Freund wrote:
> >On 2013-11-20 12:48:50 +0200, Heikki Linnakangas wrote:
> >>Looks ok for a back-patchable fix.
> >
> >Do you plan to commit this? Or who is going to?
> 
> Ok, committed.

Thanks!

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] Suggestion: Issue warning when calling SET TRANSACTION outside transaction block

[Alvaro Herrera]

Bruce Momjian escribió:

> OK, here is a patch which changes ABORT from NOTICE to WARNING, and SET
> from ERROR (which is new in 9.4) to WARNING.

I don't like that this patch changes RequireTransactionChain() from
actually requiring one, to a function that maybe requires a transaction
chain, and maybe it only complains about there not being one.  I mean,
it's like you had named the new throwError boolean as "notReally" or
something like that.  Also, the new comment paragraph is bad because it
explains who must pass true/false, instead of what's the effect of each
value (and let the callers choose which value to pass).

I would create a separate function to implement this, maybe
WarnUnlessInTransactionBlock() or something like that.  That would make
the patch a good deal smaller (because not changing existing callers).

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


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

[HACKERS] commit fest 2013-11 week 1 report

[Peter Eisentraut]

We started with

Fri Nov 15

Status Summary. Needs Review: 79, Waiting on Author: 7, Ready for Committer: 5, 
Committed: 7, Returned with Feedback: 3, Rejected: 1. Total: 102.

We are now at

Fri Nov 22

Status Summary. Needs Review: 47, Waiting on Author: 28, Ready for Committer: 
10, Committed: 18, Returned with Feedback: 3, Rejected: 3. Total: 109.

(some late arrivals, some patches split)

Progress has been quite good.

Almost all patch authors responded to my call to sign up for reviewing
someone else's patch.

20 patches are still without reviewer.  Most of those are the typical
difficult topics (indexes, replication), so now might be a good time
for experts in those areas to start picking up the remaining patches.

In the coming week, we will be following up with reviewers to send in
their first review if they haven't already done so.


-- 
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] Handling GIN incomplete splits

[Michael Paquier]

On Thu, Nov 21, 2013 at 9:44 PM, Heikki Linnakangas
 wrote:
> Here's a new version. To ease the review, I split the remaining patch again
> into two, where the first patch is just yet more refactoring.
>
> I also fixed some bugs in WAL logging and replay that I bumped into while
> testing.
Cool. Here is the review of the two remaining patches.
1) More refactoring, general remarks:
- Code compiles without warnings
- Passes make check
- If I got it correctly... this patch separates the part managing data
to be inserted from ginbtree. I can understand the meaning behind that
if we consider that GinBtree is used only to define methods and search
conditions (flags and keys). However I am wondering if this does not
make the code more complicated... Particularly the flag isDelete that
is only passed to ginxlogInsert meritates its comment IMO. Note that I
haven't read the 2nd patch when writing that :)
- With this patch, previous SELECT example takes 5.236 ms in average,
runtime does not change.
1-1) This block of code is repeated several times and should be
refactored into a single function:
/* During index build, count the new page */
if (buildStats)
{
if (btree->isData)
buildStats->nDataPages++;
else
buildStats->nEntryPages++;
}
Something with a function like that perhaps:
static void ginCountNewPage(GinBtree btree, GinStatsData *buildStats);
1-2) Could it be possible to change the variable name of
"GinBtreeEntryInsertData *entry" in entryIsEnoughSpace? entry->entry
is kind of hard to apprehend... Renaming it to either insertEntry.
Another idea would be to rename entry in GinBtreeEntryInsertData to
entryData or entryTuple.
1-3) This block of code is present two times:
+   if (!isleaf)
+   {
+   PostingItem *pitem = GinDataPageGetPostingItem(lpage, off);
+   PostingItemSetBlockNumber(pitem, updateblkno);
+   }
Should the update of a downlink to point to the next page be a
separate function?
2) post recovery cleanup:
- OK, so roughly the soul of this patch is to change the update
mechanism for a left child gin page so as the parent split is always
done first before any new data is inserted in this child. And this
ensures that we can remove the xlog cleanup mechanism for gin page
splits in the same fashion as gist... xlog redo mechanism is then
adapted according to that.
- Compilation fails becausze the flags GIN_SPLIT_ISLEAF and
GIN_SPLIT_ISDATA are missing in gin_private.h. The patch attached
fixes that though.
- With my additional patch, it passes make check, compilation shows no warnings.
- I did some tests with the patch:
-- Index creation time
vanilla: 3266.834
with the two patches: 3412.473 ms
-- Tried the new redo mechanism by simulating some server failures a
couple of times and saw no failures
- I am seeing similar run times for queries like the example used in
previous emails of this thread. So no problem on this side.
- And... Here are some comments about the code:
2-1) In ginFindParents, is the case where the stack has no parent
possible (aka the stack is the root itself)? Shouldn't this code path
check if root is NULL or not?
2-2) Not sure that this structure is in-line with the project policy:
struct
{
   BlockNumber left;
   BlockNumber right;
} children;
Why not adding a complementary structure in gin_private.h doing that?
It could be used as well in ginxlogSplit to specify a left/right
family of block numbers.
2-3) s/kepy/kept (comments of ginFinishSplit)
Other than that, the patch looks great. I particularly like the new
redo logic in ginxlog.c, the code is more easily understandable with
the split redo removed. Even if I am just a noob for this code, it is
nicely built and structured.

Regards,
-- 
Michael
diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h
index d27ff7d..382a23c 100644
--- a/src/include/access/gin_private.h
+++ b/src/include/access/gin_private.h
@@ -48,7 +48,9 @@ typedef GinPageOpaqueData *GinPageOpaque;
 #define GIN_META (1 << 3)
 #define GIN_LIST (1 << 4)
 #define GIN_LIST_FULLROW  (1 << 5) /* makes sense only on GIN_LIST 
page */
-#define GIN_INCOMPLETE_SPLIT (1 << 6)  /* page was split, but parent not 
updated */
+#define GIN_SPLIT_ISLEAF  (1 << 6)
+#define GIN_SPLIT_ISDATA  (1 << 7)
+#define GIN_INCOMPLETE_SPLIT (1 << 8)  /* page was split, but parent not 
updated */
 
 /* Page numbers of fixed-location pages */
 #define GIN_METAPAGE_BLKNO (0)

-- 
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] Data corruption issues using streaming replication on 9.0.14/9.2.5/9.3.1

[Heikki Linnakangas]

On 21.11.2013 22:55, Andres Freund wrote:

On 2013-11-20 12:48:50 +0200, Heikki Linnakangas wrote:

On 19.11.2013 16:22, Andres Freund wrote:

On 2013-11-19 15:20:01 +0100, Andres Freund wrote:

Imo something the attached patch should be done. The description I came

g> >>up with is:


 Fix Hot-Standby initialization of clog and subtrans.


Looks ok for a back-patchable fix.


Do you plan to commit this? Or who is going to?


Ok, committed.

- 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] Add \i option to bring in the specified file as a quoted literal

[Alvaro Herrera]

Amit Kapila escribió:
> On Fri, Nov 22, 2013 at 1:33 AM, Alvaro Herrera
>  wrote:

> > \ib homer ~/photos/homer.jpg
> > insert into people (name, photo) values ('Homer', :homer);
> 
>  Isn't something similar already supported as mentioned in docs:
> 
> One example use of this mechanism is to copy the contents of a file
> into a table column. First load the file into a variable and then
> interpolate the variable's value as a quoted string:
> 
> testdb=> \set content `cat my_file.txt`
> testdb=> INSERT INTO my_table VALUES (:'content');
> 
> or do you prefer an alternative without any kind of quote using \ib?

If the only use case of the feature proposed in this thread is to load
stuff from files to use as column values, then we're pretty much done,
and this patch is not needed -- except, maybe, that the `` is unlikely
to work on Windows, as already mentioned elsewhere.  But if the OP had
something else in mind, let's hear what it is.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


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

[HACKERS] Minor patch for the uuid-ossp extension

[roadrunner6]

When trying to add the extension with \i it writes an error message:
Use "CREATE EXTENSION uuid-ossp" to load this file.

Unfortunatly this does not work for extensions with dashes. Must CREATE 
EXTENSION "uuid-ossp". Proposed patch is attached.


Regards
Mario

diff -Nurb contrib.orig/uuid-ossp/uuid-ossp--1.0.sql 
contrib/uuid-ossp/uuid-ossp--1.0.sql
--- contrib.orig/uuid-ossp/uuid-ossp--1.0.sql   2013-11-22 13:48:21.588674030 
+0100
+++ contrib/uuid-ossp/uuid-ossp--1.0.sql2013-11-22 13:52:13.232387782 
+0100
@@ -1,7 +1,7 @@
 /* contrib/uuid-ossp/uuid-ossp--1.0.sql */

 -- complain if script is sourced in psql, rather than via CREATE EXTENSION
-\echo Use "CREATE EXTENSION uuid-ossp" to load this file. \quit
+\echo Use CREATE EXTENSION "uuid-ossp" to load this file. \quit

 CREATE FUNCTION uuid_nil()
 RETURNS uuid
diff -Nurb contrib.orig/uuid-ossp/uuid-ossp--unpackaged--1.0.sql 
contrib/uuid-ossp/uuid-ossp--unpackaged--1.0.sql
--- contrib.orig/uuid-ossp/uuid-ossp--unpackaged--1.0.sql   2013-11-22 
13:44:04.589862871 +0100
+++ contrib/uuid-ossp/uuid-ossp--unpackaged--1.0.sql2013-11-22 
13:52:19.480164238 +0100
@@ -1,7 +1,7 @@
 /* contrib/uuid-ossp/uuid-ossp--unpackaged--1.0.sql */

 -- complain if script is sourced in psql, rather than via CREATE EXTENSION
-\echo Use "CREATE EXTENSION uuid-ossp" to load this file. \quit
+\echo Use CREATE EXTENSION "uuid-ossp" to load this file. \quit

 ALTER EXTENSION "uuid-ossp" ADD function uuid_nil();
 ALTER EXTENSION "uuid-ossp" ADD function uuid_ns_dns();
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] UNNEST with multiple args, and TABLE with multiple funcs

[Andrew Gierth]

> "Tom" == Tom Lane  writes:

 Tom> I've committed this patch after some significant editorialization, but
 Tom> leaving the use of TABLE( ... ) syntax in-place.  If we decide that we
 Tom> don't want to risk doing that, we can change to some other syntax later.

Is this intended:

create function foo() returns setof footype language plpgsql
  as $f$ begin return next row(1,true); end; $f$;

select pg_typeof(f), row_to_json(f) from foo() with ordinality f(p,q);
 pg_typeof |   row_to_json   
---+-
 record| {"p":1,"q":true,"ordinality":1}
(1 row)

select pg_typeof(f), row_to_json(f) from foo() f(p,q);
 pg_typeof |   row_to_json
---+--
 footype   | {"a":1,"b":true}
(1 row)

-- 
Andrew (irc:RhodiumToad)


-- 
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] information schema parameter_default implementation

[Rodolfo Campero]

Review: information schema parameter_default implementation (v2)

This is a review of the patch submitted in
http://archives.postgresql.org/message-id/1384483678.5008.1.ca...@vanquo.pezone.net
(information schema parameter_default implementation).

Previous review from Amit Khandekar covers technical aspects:
http://www.postgresql.org/message-id/CACoZds0eB3-yZ+pVLp9Sf5Xs_9xsDZ=jdp1d83ra-hjvjjo...@mail.gmail.com

Submission review
=
 * Is the patch in a patch format which has context? (eg: context diff
format)
Yes

 * Does it apply cleanly to the current git master?
I had to apply "fromdos" to remove trailing whitespace.
After that, the patch applies cleanly to HEAD.
Make builds without warnings, except for:

In file included from gram.y:13675:0:
scan.c: In function ‘yy_try_NUL_trans’:
scan.c:10185:23: warning: unused variable ‘yyg’ [-Wunused-variable]

but from previous messages in this mailing list I think that's unrelated to
this patch and normal.
The regression tests all pass successfully against the new patch.

 * Does it include reasonable tests, necessary doc patches, etc?
Yes

Usability review

 * Does the patch actually implement that?
The patch implements the column "parameter_default" of information schema
view "parameters", defined in the SQL:2011 standard.
I could not get a hand to the spec, but I found a document where it is
mentioned: http://msdn.microsoft.com/en-us/library/jj191733(v=sql.105).aspx

 * Do we want that?
I think we do, as it is defined in the spec.

 * Do we already have it?
No

 * Does it follow SQL spec, or the community-agreed behavior?
SQL:2011.

 * Does it include pg_dump support (if applicable)?
N/A

 * Are there dangers?
None AFAICS.

 * Have all the bases been covered?
Yes.

Feature test

 * Does the feature work as advertised?
Yes

 * Are there corner cases the author has failed to consider?
None that I can see.

 * Are there any assertion failures or crashes?
No

Performance review
==
N/A

Coding review
=
I'm not skilled enough to do a code review; see previous review from Amit:
http://www.postgresql.org/message-id/CACoZds0eB3-yZ+pVLp9Sf5Xs_9xsDZ=jdp1d83ra-hjvjjo...@mail.gmail.com



2013/11/21 Peter Eisentraut 

> On 11/20/13, 8:39 PM, Rodolfo Campero wrote:
> > 2013/11/20 Peter Eisentraut mailto:pete...@gmx.net>>
> >
> > Updated patch
> >
> >
> > I can't apply the patch; maybe I'm doing something wrong?
>
> It looks like you are not in the right directory.
>
>


-- 
Rodolfo Campero
Anachronics S.R.L.
Tel: (54 11) 4899 2088
rodolfo.camp...@anachronics.com
http://www.anachronics.com

Re: [HACKERS] [PATCH] Store Extension Options

[Fabrízio de Royes Mello]

On Thu, Nov 21, 2013 at 11:06 AM, Robert Haas  wrote:
>
> On Wed, Nov 20, 2013 at 9:35 PM, Fabrízio de Royes Mello
>  wrote:
> >> > So, with this patch we can do that:
> >> >
> >> > ALTER TABLE foo
> >> >SET (ext.somext.do_replicate=true);
> >> >
> >> > When 'ext' is the fixed prefix, 'somext' is the extension name,
> >> > 'do_replicate' is the
> >> > extension option and 'true' is the value.
> >>
> >> This doesn't seem like a particular good choice of syntax;
> >
> > What's your syntax suggestion?
>
> I dunno, but I doubt that hardcoding ext as an abbreviation for
> extension is a good decision.  I also doubt that any fixed prefix is a
> good decision.
>

I use this form to simplify implementation and not change sql syntax, but
we can discuss another way or syntax.


> >> and I also have my doubts about the usefulness of the feature.
> >
> > This feature can be used for replication solutions, but also can be
used for
> > any extension that need do some specific configuration about tables,
> > attributes and/or indexes.
>
> So, create your own configuration table with a column of type regclass.
>

This can be a solution, but with a config table we have some problems:
a) no dependency tracking (pg_depend)
b) no correct locking
c) no relcache
d) harder to do correctly for columns

Regards,

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

Re: [HACKERS] Data corruption issues using streaming replication on 9.0.14/9.2.5/9.3.1

[Heikki Linnakangas]

On 19.11.2013 16:20, Andres Freund wrote:

On 2013-11-18 23:15:59 +0100, Andres Freund wrote:

Afaics it's likely a combination/interaction of bugs and fixes between:
* the initial HS code
* 5a031a5556ff83b8a9646892715d7fef415b83c3
* f44eedc3f0f347a856eea8590730769125964597


Yes, the combination of those is guilty.

Man, this is (to a good part my) bad.


But that'd mean nobody noticed it during 9.3's beta...


It's fairly hard to reproduce artificially since a) there have to be
enough transactions starting and committing from the start of the
checkpoint the standby is starting from to the point it does
LogStandbySnapshot() to cross a 32768 boundary b) hint bits often save
the game by not accessing clog at all anymore and thus not noticing the
corruption.
I've reproduced the issue by having an INSERT ONLY table that's never
read from. It's helpful to disable autovacuum.


For the archive, here's what I used to reproduce this. It creates master 
and a standby, and also uses an INSERT only table. To make it trigger 
more easily, it helps to insert sleeps in CreateCheckpoint(), around the 
LogStandbySnapshot() call.


- Heikki


test-hot-standby-bug.sh
Description: Bourne shell script

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

Re: [HACKERS] PL/Python: domain over array support

[Marko Kreen]

On Fri, Nov 22, 2013 at 08:45:56AM -0200, Rodolfo Campero wrote:
> There are other cosmetic changes in this patch, wrt previous version (not
> preexistent code):
>  * adjusted alignment of variable name "rv" in line 12
>  * reworded comment in line 850, resulting in more than 80 characters, so I
> splitted the comment into a multiline comment following the surrounding
> style.

Good.

One more thing - please update Python 3 regtests too.

-- 
marko



-- 
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] Cpu usage 100% on slave. s_lock problem.

[Heikki Linnakangas]

On 21.11.2013 21:37, Merlin Moncure wrote:

On Thu, Nov 21, 2013 at 10:37 AM, Heikki Linnakangas

In my patch, I put the barrier inside the if (!LocalRecoveryInProgress)
block. That codepath can only execute once in a backend, so performance is
not an issue there. Does that look sane to you?


oh right -- certainly!


Ok, commmited.

- 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] PL/Python: domain over array support

[Rodolfo Campero]

Marko,

2013/11/22 Marko Kreen 

> On Sat, Oct 26, 2013 at 11:17:19AM -0200, Rodolfo Campero wrote:
> > The attached patch add support of domains over arrays to PL/Python (eg:
> > CREATE DOMAIN my_domain AS integer[]).
> >
> > Basically it just uses get_base_element_type instead of get_element_type
> > in plpy_typeio.c, and uses domain_check before returning a sequence as
> > array in PLySequence_ToArray whenever appropriate.
>
> Generally looks fine.  Please lose the C++ comments though, this style
> is not used in Postgres sources.
>

Done.


> > There's one line I'm not sure about; I modified a switch statement (line
> > 427):
> > switch (element_type ? element_type : getBaseType(arg->typoid))
> > The rationale is that when element_type is set, it is already a base
> type,
> > because there is no support of arrays of domains in PostgreSQL, but this
> > may not held true in the future.
>
> Was there any actual need to modify that?  Or was it just performance
> optimization?  ATM it creates asymmetry between PLy_output_datum_func2
> and PLy_input_datum_func2.
>
> If it's just performace optimization, then it should be done in both
> functions, but seems bad idea to do it in this patch.  So I think
> it's better to leave it out.
>
>
There was no actual need to modify that, so I dropped that change in this
new patch.

There are other cosmetic changes in this patch, wrt previous version (not
preexistent code):
 * adjusted alignment of variable name "rv" in line 12
 * reworded comment in line 850, resulting in more than 80 characters, so I
splitted the comment into a multiline comment following the surrounding
style.

Thanks for your review.
diff --git a/src/pl/plpython/expected/plpython_types.out b/src/pl/plpython/expected/plpython_types.out
index 91106e0..785ffca 100644
--- a/src/pl/plpython/expected/plpython_types.out
+++ b/src/pl/plpython/expected/plpython_types.out
@@ -664,6 +664,34 @@ SELECT * FROM test_type_conversion_array_error();
 ERROR:  return value of function with array return type is not a Python sequence
 CONTEXT:  while creating return value
 PL/Python function "test_type_conversion_array_error"
+CREATE DOMAIN ordered_pair_domain AS integer[] CHECK (array_length(VALUE,1)=2 AND VALUE[1] < VALUE[2]);
+CREATE FUNCTION test_type_conversion_array_domain(x ordered_pair_domain) RETURNS ordered_pair_domain AS $$
+plpy.info(x, type(x))
+return x
+$$ LANGUAGE plpythonu;
+SELECT * FROM test_type_conversion_array_domain(ARRAY[0, 100]::ordered_pair_domain);
+INFO:  ([0, 100], )
+CONTEXT:  PL/Python function "test_type_conversion_array_domain"
+ test_type_conversion_array_domain 
+---
+ {0,100}
+(1 row)
+
+SELECT * FROM test_type_conversion_array_domain(NULL::ordered_pair_domain);
+INFO:  (None, )
+CONTEXT:  PL/Python function "test_type_conversion_array_domain"
+ test_type_conversion_array_domain 
+---
+ 
+(1 row)
+
+CREATE FUNCTION test_type_conversion_array_domain_check_violation() RETURNS ordered_pair_domain AS $$
+return [2,1]
+$$ LANGUAGE plpythonu;
+SELECT * FROM test_type_conversion_array_domain_check_violation();
+ERROR:  value for domain ordered_pair_domain violates check constraint "ordered_pair_domain_check"
+CONTEXT:  while creating return value
+PL/Python function "test_type_conversion_array_domain_check_violation"
 ---
 --- Composite types
 ---
diff --git a/src/pl/plpython/plpy_typeio.c b/src/pl/plpython/plpy_typeio.c
index caccbf9..0a2307a 100644
--- a/src/pl/plpython/plpy_typeio.c
+++ b/src/pl/plpython/plpy_typeio.c
@@ -373,7 +373,7 @@ PLy_output_datum_func2(PLyObToDatum *arg, HeapTuple typeTup)
 	arg->typioparam = getTypeIOParam(typeTup);
 	arg->typbyval = typeStruct->typbyval;
 
-	element_type = get_element_type(arg->typoid);
+	element_type = get_base_element_type(arg->typoid);
 
 	/*
 	 * Select a conversion function to convert Python objects to PostgreSQL
@@ -427,7 +427,8 @@ static void
 PLy_input_datum_func2(PLyDatumToOb *arg, Oid typeOid, HeapTuple typeTup)
 {
 	Form_pg_type typeStruct = (Form_pg_type) GETSTRUCT(typeTup);
-	Oid			element_type = get_element_type(typeOid);
+	/* It's safe to handle domains of array types as its base array type. */
+	Oid			element_type = get_base_element_type(typeOid);
 
 	/* Get the type's conversion information */
 	perm_fmgr_info(typeStruct->typoutput, &arg->typfunc);
@@ -808,6 +809,7 @@ static Datum
 PLySequence_ToArray(PLyObToDatum *arg, int32 typmod, PyObject *plrv)
 {
 	ArrayType  *array;
+	Datum   rv;
 	int			i;
 	Datum	   *elems;
 	bool	   *nulls;
@@ -844,8 +846,15 @@ PLySequence_ToArray(PLyObToDatum *arg, int32 typmod, PyObject *plrv)
 
 	lbs = 1;
 	array = construct_md_array(elems, nulls, 1, &len, &lbs,
-			   get_element_type(arg->typoid), arg->elm->typlen, arg->elm->typbyval, arg->elm->typalign);
-	return PointerGetDatum(array);
+			   get_base_element_type(arg->typoid), arg->elm->typlen, arg->elm->typbyval, arg->elm->typalign);
+	/*
+	 * If the result type is a 

Re: [HACKERS] Can we trust fsync?

[Florian Weimer]

On 11/21/2013 12:45 AM, Craig Ringer wrote:

I'm really concerned by this post on Linux's fsync and disk flush behaviour:

http://milek.blogspot.com.au/2010/12/linux-osync-and-write-barriers.html

and seeking opinions from folks here who've been deeply involved in
write reliability work.


With ext4 and XFS on plain/LVM/md block devices, this issue should 
really be a thing of the past.  I think the kernel folks would treat 
this as bugs nowadays, too.


--
Florian Weimer / Red Hat Product Security Team


--
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] Get more from indices.

[Etsuro Fujita]

Kyotaro HORIGUCHI wrote:
> Hello. I found a bug(?) in thsi patch as I considered on another patch.

> In truncate_useless_pathkeys, if query_pathkeys is longer than pathkeys
made
> from index columns old patch picked up the latter as IndexPath's pathkeys.

> But the former is more suitable according to the context here.

> the attched pathkey_and_uniqueindx_v4_20131122.patch is changed as
follows.

>  - Rebased to current master.

>  - truncate_useless_pathkeys returns root->query_pathkeys when
>the index is fully-ordered and query_pathkeys contains the
>pathkeys made from index columns.

OK, I'd like to look at this version of the patch more closely, then.

Thanks,

Best regards,
Etsuro Fujita



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

Re: [HACKERS] PL/Python: domain over array support

[Marko Kreen]

On Sat, Oct 26, 2013 at 11:17:19AM -0200, Rodolfo Campero wrote:
> The attached patch add support of domains over arrays to PL/Python (eg:
> CREATE DOMAIN my_domain AS integer[]).
> 
> Basically it just uses get_base_element_type instead of get_element_type
> in plpy_typeio.c, and uses domain_check before returning a sequence as
> array in PLySequence_ToArray whenever appropriate.

Generally looks fine.  Please lose the C++ comments though, this style
is not used in Postgres sources.

> There's one line I'm not sure about; I modified a switch statement (line
> 427):
> switch (element_type ? element_type : getBaseType(arg->typoid))
> The rationale is that when element_type is set, it is already a base type,
> because there is no support of arrays of domains in PostgreSQL, but this
> may not held true in the future.

Was there any actual need to modify that?  Or was it just performance
optimization?  ATM it creates asymmetry between PLy_output_datum_func2
and PLy_input_datum_func2.

If it's just performace optimization, then it should be done in both
functions, but seems bad idea to do it in this patch.  So I think
it's better to leave it out.

-- 
marko



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