Re: [HACKERS] Triggers on foreign tables

2014-03-22 Thread Noah Misch
On Tue, Mar 18, 2014 at 09:31:06AM +0100, Ronan Dunklau wrote:
> Le mardi 18 mars 2014 03:54:19 Kouhei Kaigai a écrit :
> > > (1) To acquire the old tuple for UPDATE/DELETE operations, the patch
> > > closely
>  parallels our handling for INSTEAD OF triggers on views.  It
> > > adds a wholerow resjunk attribute, from which it constructs a HeapTuple
> > > before calling a trigger function.  This loses the system columns, an
> > > irrelevant
> > > consideration for views but meaningful for foreign tables.  postgres_fdw
> > > maintains the "ctid" system column (t_self), but triggers will always see
> > > an invalid t_self for the old tuple.  One way to fix this is to pass
> > > around
>  the old tuple data as ROW(ctid, oid, xmin, cmin, xmax, cmax,
> > > tableoid, wholerow).  That's fairly close to sufficient, but it doesn't
> > > cover t_ctid. Frankly, I would like to find a principled excuse to not
> > > worry about making foreign table system columns accessible from triggers.
> > >  Supporting system columns dramatically affects the mechanism, and what
> > > trigger is likely to care?  Unfortunately, that argument seems too weak. 
> > > Does anyone have a cleaner idea for keeping track of the system column
> > > values or a stronger argument for not bothering?
> > > 
> > 
> > Regarding to the first suggestion,
> > I think, it is better not to care about system columns on foreign tables,
> > because it fully depends on driver's implementation whether FDW fetches
> > "ctid" from its data source, or not.
> > Usually, system columns of foreign table, except for tableoid, are
> > nonsense.
>  Because of implementation reason, postgres_fdw fetches "ctid" of
> > remote tables on UPDATE / DELETE, it is not a common nature for all FDW
> > drivers. For example, we can assume an implementation that uses primary key
> > of remote table to identify the record to be updated or deleted. In this
> > case, local "ctid" does not have meaningful value.
> > So, fundamentally, we cannot guarantee FDW driver returns meaningful "ctid"
> > or other system columns.
> > 
> 
> I agree, I think it is somewhat clunky to have postgres_fdw use a feature 
> that 
> is basically meaningless for other FDWs. Looking at some threads in this 
> list, 
> it confused many people.

My own reasoning for accepting omission of system columns is more along the
lines of Robert's argument.  Regardless, three folks voting to do so and none
against suffices for me.  I documented the system columns limitation, made the
returningList change I mentioned, and committed the patch.

> This is off-topic, but maybe we could devise an API allowing for local 
> "system 
> attributes" on foreign table. This would allow FDWs to carry attributes that 
> weren't declared as part of the table definition. This could then be used for 
> postgres_fdw ctid, as well as others foreign data wrappers equivalent of an 
> implicit "tuple id".

We could, but I discourage it.  System columns are a legacy feature; I doubt
we'd choose that design afresh today.  On the off-chance that you need the
value of a remote system column, you can already declare an ordinary foreign
table column for it.  I raised the issue because it's inconsistent for
RETURNING to convey system columns while tg_trigtuple/tg_newtuple do not, not
because acquiring system columns from foreign tables is notably useful.

Thanks,
nm

-- 
Noah Misch
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] equalTupleDescs() ignores ccvalid/ccnoinherit

2014-03-22 Thread Noah Misch
On Fri, Mar 21, 2014 at 06:59:05PM +, Simon Riggs wrote:
> On 21 March 2014 18:26, Robert Haas  wrote:
> 
> >> Given the minor symptoms in released versions, I lean against a back-patch.
> >
> > FWIW, I'd lean toward a back-patch.  It's probably not a big deal
> > either way, but I have a hard time seeing what risk we're avoiding by
> > not back-patching, and it seems potentially confusing to leave
> > known-wrong logic floating around in older branches.
> 
> Agreed. It could lead to some other bug by not fixing it.

Fair enough.  I've back-patched it.

-- 
Noah Misch
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: Fwd: [HACKERS] Proposal: variant of regclass

2014-03-22 Thread Amit Kapila
On Sat, Mar 22, 2014 at 1:17 PM, Amit Kapila  wrote:
> On Wed, Feb 26, 2014 at 11:56 AM, Yugo Nagata  wrote:
>> Thanks for your a lot of comments. I revised the patch according to
>> comments from Robert Haas and Marti Raudsepp.
>
> I have started looking into this patch and below are my
> initial findings:
>
> 1. Dependency is not recorded when to_regclass is used,
> however it is recorded when ::regclass is used. Below is
> test to demonstrate this point. This change in behaviour is
> neither discussed nor mentioned in docs along with patch.

I think this is expected as per current design, because for
functions, it will create dependency on function (funcid), but
not on it's argument values. So I think for this behaviour, we might
need to mention in docs that to_regclass() and other newly added
functions will behave differently for creation of dependencies.

Anyone has any objection for this behaviour difference between
usage of ::regclass and to_regclass()?


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] Useless "Replica Identity: NOTHING" noise from psql \d

2014-03-22 Thread Bruce Momjian
On Tue, Dec 17, 2013 at 09:37:09AM -0500, Robert Haas wrote:
> > Patch attached.
> 
> I vote for showing it only with +, but regardless of whether the value
> matches the expected default.  I'd keep the relkind test, though,
> because I think I noticed that it currently shows up for indexes,
> which is dumb.

Is this the patch you had in mind?  I kept the pg_catalog filter.  Do we
want to always show the replica identity line for \d+?

test=> \d+ test
 Table "public.test"
 Column |  Type   | Modifiers | Storage | Stats target | Description
+-+---+-+--+-
 x  | integer |   | plain   |  |
Replica Identity: full
Has OIDs: no

I used lower-case for the value, rather than all-caps.

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

  + Everyone has their own god. +
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
new file mode 100644
index a194ce7..a75fc82
*** a/src/bin/psql/describe.c
--- b/src/bin/psql/describe.c
*** describeOneTableDetails(const char *sche
*** 2345,2358 
  			printTableAddFooter(&cont, buf.data);
  		}
  
! 		if ((tableinfo.relkind == 'r' || tableinfo.relkind == 'm') &&
! 			tableinfo.relreplident != 'd' && tableinfo.relreplident != 'i')
  		{
  			const char *s = _("Replica Identity");
  
  			printfPQExpBuffer(&buf, "%s: %s",
  			  s,
! 			  tableinfo.relreplident == 'n' ? "NOTHING" : "FULL");
  			printTableAddFooter(&cont, buf.data);
  		}
  
--- 2345,2358 
  			printTableAddFooter(&cont, buf.data);
  		}
  
! 		if (verbose && (tableinfo.relkind == 'r' || tableinfo.relkind == 'm') &&
! 			strcmp(schemaname, "pg_catalog") != 0)
  		{
  			const char *s = _("Replica Identity");
  
  			printfPQExpBuffer(&buf, "%s: %s",
  			  s,
! 			  tableinfo.relreplident == 'n' ? "nothing" : "full");
  			printTableAddFooter(&cont, buf.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] jsonb status

2014-03-22 Thread Peter Geoghegan
On Sat, Mar 22, 2014 at 5:22 PM, Bruce Momjian  wrote:
> What did you decide about hashing values in indexes vs. putting them in
> literally?

There are two GIN opclasses supplied. There is a default, which
supports more operators (various "existence" operators - see the
documentation). There is an alternative called jsonb_hash_ops that
only supports containment, and performs considerably better than the
default. Containment *is* the compelling operator to support, though -
you can do rather a lot with it. This must be what you're referring
to, since I recall you blogged about the response it got at pgConf.EU.
Both are available.


-- 
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] jsonb status

2014-03-22 Thread Bruce Momjian
On Sat, Mar 22, 2014 at 01:53:06PM -0700, Peter Geoghegan wrote:
> Attached is v12. I think I've brought this as far as I can.
> 
> This is mostly just bug fixes, and some additional refactoring. I've
> incorporated Andres' feedback. The only points that I think worth
> noting are:
> 
> * The documentation has been significantly expanded to discuss
> "containment" further, since it's a significant part of the feature.
> This could probably use some polishing and general scrutiny, which is
> something that Andrew may consider. I didn't have time to go over it
> to the extent that I'd prefer.
> 
> * I altered containment semantics slightly. Now, it is not possible
> for a "raw scalar" to contain a single-element array of the same
> scalar, while the inverse is still possible (raw scalars still contain
> themselves too). This made sense to me, and is consistent with the
> behavior of the B-Tree operator class, where a raw scalar is not equal
> to a single-element array of the same scalar. Rather, array is greater
> than the scalar on the basis of its type alone, as at every other
> nesting level. The fact that an array can contain a raw scalar is
> explicitly presented as an exception to the general principle that
> containment needs to be at the same nesting level.
> 
> I'm not going to go into the details of the bugs fixed, since no one
> reported them here, and since they're trivial in nature. For full
> details, you can review the log of our publicly accessible feature
> branch.

What did you decide about hashing values in indexes vs. putting them in
literally?

-- 
  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] Partial index locks

2014-03-22 Thread Thom Brown
On 22 March 2014 16:28, Jim Nasby  wrote:
> On 3/21/14, 7:59 PM, Vik Fearing wrote:
>>
>> On 03/22/2014 01:43 AM, Thom Brown wrote:
>>>
>>> Hi,
>>>
>>> I've created a table with 1000 partial indexes.  Each one matches
>>> exactly one row based on the predicate WHERE id = .
>>>
>>> However, when I perform an UPDATE of a single row in a transaction,
>>> I've noticed that all those partial indexes show up in pg_locks with
>>> RowExclusiveLock.
>>>
>>> Only 2 of those indexes have a reference to the row: the primary key
>>> and a single partial index.
>>>
>>> Is it necessary for a partial index that doesn't include the row to be
>>> involved in locking?
>>
>>
>> What if the update puts the row into one of the other indexes?
>
>
> Also, why are you doing this in the first place? I'm guessing you measured
> some non-trivial performance improvement from doing this; could you share
> that with us?

Heh, no.  I was just experimenting with various things, and also
trying to break stuff.

-- 
Thom


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


Re: [HACKERS] psql blows up on BOM character sequence

2014-03-22 Thread Jim Nasby

On 3/21/14, 4:54 PM, Tom Lane wrote:

Merlin Moncure  writes:

There is no way for psql to handle that case though unless you'd strip
*all* BOMs encountered.  Compounding this problem is that there's no
practical way AFAIK to send multiple file to psql via single command
line invocation.  If you pass multiple -f arguments all but one is
ignored.


Well, that seems like a solvable but rather independent problem.
I guess one issue is how you'd define the meaning of --single ...
one transaction per run, or one per file?


Well, if you're catting multiple files into psql -1, you'd get all the files in 
one transaction, right? So I'd say that's what should happen.
--
Jim C. Nasby, Data Architect   j...@nasby.net
512.569.9461 (cell) http://jim.nasby.net


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


Re: [HACKERS] ALTER TABLE lock strength reduction patch is unsafe

2014-03-22 Thread Jim Nasby

On 2/26/14, 9:15 AM, Simon Riggs wrote:

On 26 February 2014 13:38, Andres Freund  wrote:

>Hi,
>
>On 2014-02-26 07:32:45 +, Simon Riggs wrote:

>> >* This definitely should include isolationtester tests actually
>> >   performing concurrent ALTER TABLEs. All that's currently there is
>> >   tests that the locklevel isn't too high, but not that it actually works.

>>
>>There is no concurrent behaviour here, hence no code that would be
>>exercised by concurrent tests.

>
>Huh? There's most definitely new concurrent behaviour. Previously no
>other backends could have a relation open (and locked) while it got
>altered (which then sends out relcache invalidations). That's something
>that should be tested.

It has been. High volume concurrent testing has been performed, per
Tom's original discussion upthread, but that's not part of the test
suite.
For other tests I have no guide as to how to write a set of automated
regression tests. Anything could cause a failure, so I'd need to write
an infinite set of tests to prove there is no bug*somewhere*. How
many tests are required? 0, 1, 3, 30?


ISTM that we don't want hand-written tests here, but rather generated tests 
that actually hit all potential cases. Obviously we'd never run that as part of 
normal reqression, but farm animals certainly could.
--
Jim C. Nasby, Data Architect   j...@nasby.net
512.569.9461 (cell) http://jim.nasby.net


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


Re: [HACKERS] Partial index locks

2014-03-22 Thread Jim Nasby

On 3/21/14, 7:59 PM, Vik Fearing wrote:

On 03/22/2014 01:43 AM, Thom Brown wrote:

Hi,

I've created a table with 1000 partial indexes.  Each one matches
exactly one row based on the predicate WHERE id = .

However, when I perform an UPDATE of a single row in a transaction,
I've noticed that all those partial indexes show up in pg_locks with
RowExclusiveLock.

Only 2 of those indexes have a reference to the row: the primary key
and a single partial index.

Is it necessary for a partial index that doesn't include the row to be
involved in locking?


What if the update puts the row into one of the other indexes?


Also, why are you doing this in the first place? I'm guessing you measured some 
non-trivial performance improvement from doing this; could you share that with 
us?
--
Jim C. Nasby, Data Architect   j...@nasby.net
512.569.9461 (cell) http://jim.nasby.net


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


Re: [HACKERS] psql blows up on BOM character sequence

2014-03-22 Thread Jim Nasby

On 3/21/14, 8:13 PM, David E. Wheeler wrote:

On Mar 21, 2014, at 2:16 PM, Andrew Dunstan  wrote:


Surely if it were really a major annoyance, someone would have sent code to fix 
it during the last 4 years and more since the above.

I suspect it's a minor annoyance :-)

But by all means add it to the TODO list if it's not there already.


I have cleaned up many a BOM added to files that made psql blow up. I think 
PGAdmin III was a culprit, though I’m not sure (I don’t use, it, cleaned up 
after coworkers who do).


Yes, my coworker that figured out what the problem was said the culprit here is 
actually pgAdmin. :(
--
Jim C. Nasby, Data Architect   j...@nasby.net
512.569.9461 (cell) http://jim.nasby.net


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


Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-22 Thread Piotr Stefaniak

On 03/22/2014 04:00 PM, Tom Lane wrote:

On the other side, coding with the explicit cast helps guard against far
more dangerous coding errors, which the compiler will*not*  help you with.
What if myextra is actually of type "int64 *"?
Indeed, neither "gcc -Wall -Wextra -std=c89 -pedantic" nor "clang 
-Weverything -Wno-shadow -std=c89 -pedantic" issues a warning in such 
case. "clang --analyze", however, does. Perhaps TenDRA would, if it ever 
worked.


This message is meant to be merely informative, since I've put some 
effort into this test. I'm not trying to argue.
#include 

typedef long int int64;

int main(void)
{
  int  *myextra;

  /* with explicit casting */
  myextra = (int *) malloc(sizeof (int));
free(myextra);

  /* with no explicit casting */
  myextra = malloc(sizeof (int));
free(myextra);
  
  /* myextra now becomes int64 */
  {
int64 *myextra;

/* with explicit casting */
myextra = (int *) malloc(sizeof (int)); /* [1], [2]. and [3] warn here */
  free(myextra);

/* with no explicit casting */
myextra = malloc(sizeof (int)); /* Only [3] warns here */
  free(myextra);
  }

  return 0;
}

/*
 1: gcc 4.8.2: gcc -Wall -Wextra -std=c89 -pedantic /tmp/test.c
 2: clang 3.5.0: clang -Weverything -Wno-shadow -std=c89 -pedantic /tmp/test.c
 3: clang 3.5.0: clang --analyze /tmp/test.c
 */

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


Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-22 Thread Tom Lane
Piotr Stefaniak  writes:
> Apart from what the page says, I also think of casting malloc() as bad 
> style and felt the need to bring this up.

Well, that's a value judgement I don't happen to agree with.  Yeah, it'd
be better if the language design were such that we could avoid explicit
casting everywhere, but in this context casting is less risky than not
casting.

> So perhaps this alternative:
>myextra = malloc(sizeof *myextra);

[ shrug... ]  That's about a wash for this exact use case, but it gets
messy as soon as the lefthand side is anything more complicated than a
simple variable name.  And it doesn't scale to cases where the malloc
result isn't directly assigned to anything --- for example, what if
you want to pass the result of palloc() directly to some other
function, or return it from the current function?

The bigger picture though is that the style with the explicit cast is
already extremely widely used in Postgres.  That being the case,
conforming to project style is better than using some inconsistent
convention, regardless of your personal views about whether there's a
better way to do it.

regards, tom lane


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


Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-22 Thread Piotr Stefaniak

On 03/22/2014 04:00 PM, Tom Lane wrote:

That argument is entirely bogus, as it considers only one possible way
in which the call could be wrong; a way that is of very low probability
in PG usage, since we include  in our core headers.  Besides
which, as noted in the page itself, most modern compilers would warn
anyway if you forgot the inclusion.

Apart from what the page says, I also think of casting malloc() as bad 
style and felt the need to bring this up. But since you pointed out why 
you don't want to remove the cast, I withdraw my previous suggestion.



On the other side, coding with the explicit cast helps guard against far
more dangerous coding errors, which the compiler will *not* help you with.
What if myextra is actually of type "int64 *"?  In that case you probably
meant to make enough space for an int64 not an int.  But without the cast,
you won't be told you did anything wrong.  This is a particular hazard if
you change your mind later on about the type of myextra.  (A colleague
at Salesforce got burnt in exactly that way, just a couple days ago.)


So perhaps this alternative:
  myextra = malloc(sizeof *myextra);


PS.
Coding style matters to me, but I was and still am far from insisting on 
anything.



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


Re: [HACKERS] Inheritance of foregn key constraints.

2014-03-22 Thread Tom Lane
Andrzej Mazurkiewicz  writes:
>> So in other words, somebody could (accidentally or maliciously) break the
>> constraint by dropping one of its implementation triggers.  I doubt that's
>> acceptable.

> The present postgres behavior ALLOWS accidental or malicious break the 
> constraint by dropping one of its implementation triggers. Please ref. to the 
> following example.
> The following script has been run by the postgres user.

Well, right there you lost me, because superusers are exempt from all
permissions checks by definition; and in particular, direct manipulations
of the system catalogs by superusers are always out of scope for
discussions of what the system should try to protect itself against.
(Try "delete from pg_proc;" in a scratch database sometime.)

My point is that without the internal dependency, a normal user could do
standard SQL commands (ie DROP TRIGGER) and break the FK that way.
That's the case that's not acceptable.

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] Partial index locks

2014-03-22 Thread Thom Brown
On 22 March 2014 15:04, Tom Lane  wrote:
> Thom Brown  writes:
>> On 22 March 2014 05:32, Tom Lane  wrote:
>>> Yes.  You can't determine whether the index needs to get a new entry
>>> without examining its metadata, and that's what the lock is mainly about.
>
>> I see.  Why does this apply to deletes too?
>
> The executor doesn't take locks on indexes for a delete.  I think the
> planner probably does, though, since it wants to look at all the indexes
> to see if any can be used to satisfy WHERE searches.
>
> Possibly it would be worth hacking the planner to only take
> AccessShareLock not RowExclusiveLock on target indexes in DELETE.
> I can't get very excited about that though; in what circumstances
> would it actually make a difference?

Well I wasn't looking for things to optimise, so much as trying to
understand the logic behind the existing behaviour.  But thanks for
the explanation.

-- 
Thom


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


Re: [HACKERS] Partial index locks

2014-03-22 Thread Tom Lane
Thom Brown  writes:
> On 22 March 2014 05:32, Tom Lane  wrote:
>> Yes.  You can't determine whether the index needs to get a new entry
>> without examining its metadata, and that's what the lock is mainly about.

> I see.  Why does this apply to deletes too?

The executor doesn't take locks on indexes for a delete.  I think the
planner probably does, though, since it wants to look at all the indexes
to see if any can be used to satisfy WHERE searches.

Possibly it would be worth hacking the planner to only take
AccessShareLock not RowExclusiveLock on target indexes in DELETE.
I can't get very excited about that though; in what circumstances
would it actually make a difference?

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] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-22 Thread Tom Lane
Piotr Stefaniak  writes:
>>> +myextra = (int *) malloc(sizeof(int));

> Please consider not casting malloc(). See 

That code is per project style, and should stay that way.

> http://c-faq.com/malloc/mallocnocast.html

That argument is entirely bogus, as it considers only one possible way
in which the call could be wrong; a way that is of very low probability
in PG usage, since we include  in our core headers.  Besides
which, as noted in the page itself, most modern compilers would warn
anyway if you forgot the inclusion.

On the other side, coding with the explicit cast helps guard against far
more dangerous coding errors, which the compiler will *not* help you with.
What if myextra is actually of type "int64 *"?  In that case you probably
meant to make enough space for an int64 not an int.  But without the cast,
you won't be told you did anything wrong.  This is a particular hazard if
you change your mind later on about the type of myextra.  (A colleague
at Salesforce got burnt in exactly that way, just a couple days ago.)

So, general policy around here is that malloc and palloc calls should look
like

ptr = (foo *) malloc(n * sizeof(foo));

so that there's a direct, visible connection between the size calculation
and the type of the resulting pointer.

I'm aware that there are some places in the code that fail to do this,
but they are not models to emulate.

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] Inheritance of foregn key constraints.

2014-03-22 Thread Andrzej Mazurkiewicz
Good Morning.

1. At the beginning some explanations.

I am a lazy person that tries not to reinvent a wheel.
So I try to use postgres way of automatic processing, i. e. automatic removing 
dependent objects (which I consider an elegant solution and I really like it). 
A a result, I have used the pg_depend table to force to remove dependent 
entries.

2. At the moment the following behavior is a standard one for postgres.
- a child table (inheriting form a parent table(s) no FK) can be dropped;
- a referred table (master) can be freely dropped with a CASCADE option 
(causing dropping of the FK);
- a referring table (detail) can be freely dropped (causing automatic dropping 
of the FK);
- a CHECK constraint is inherited and the inheritance can be removed freely 
although leaving the CHECK constraint (no FK);
- an inherited table with CHECK constraint can be freely dropped (no FK);
- inheritance can be added for existing tables and it can be removed (no FK).

3. The following decisions should be taken for the FK inheritance (partly 
common issues, however  I try to be precise).
- (GENERAL statement) Are modifications of a master side hierarchy (a referred 
side) allowed without dropping the FK?
- (GENERAL statement) Are modifications of a detail side hierarchy (a referred 
side) allowed without dropping the FK?
- Is detaching childs allowed in the master and detail hierarchy without 
dropping the FK?
- Is dropping tables allowed in the master and detail hierarchy without 
dropping the FK?
- Is adding inheritance allowed to the master and detail hierarchies without 
dropping the FK?
- Is creating inheriting tables allowed in the master and detail hierarchies 
without dropping the FK?

It would be good if the decisions were consistent with the existing behavior.

The consequences of the decisions are rather far going. For large databases 
adding the FK constraint might last hours or days or perhaps weeks.

For my databases, although such modification would last hours and sometimes I 
have strange and changing ideas - I can live with those hours.

Personally I would vote that the above modifications SHOULD BE ALLOWED. Simply, 
because we do not drop the whole master or detail hierarchy but modify it and 
it gives certain flexibility to manipulating the schema.

The above flexibility is similar to adding inheritance to the existing tables 
and removing inheritance for them. We do not need to create another inherited 
table and to move data into it from the existing table.

3. Perhaps , after making the above decisions, a discussion about an 
implementing changes should be continued.

4. 
> > My patch need one change that might be of significance.
> > A type of the depencencies (pg_depend) among the FK constraint
> > (pg_constraint) and the corresponding "RI_ConstraintTrigger" triggers has
> > to be changed from DEPENDENCY_INTERNAL to DEPENDENCY_AUTO.
> 
> So in other words, somebody could (accidentally or maliciously) break the
> constraint by dropping one of its implementation triggers.  I doubt that's
> acceptable.

The present postgres behavior ALLOWS accidental or malicious break the 
constraint by dropping one of its implementation triggers. Please ref. to the 
following example.

The following script has been run by the postgres user.

CREATE DATABASE lipa;
\c lipa
CREATE TABLE master (master_a int, CONSTRAINT pk_master PRIMARY KEY 
(master_a));
CREATE TABLE detail (master_a int, detail_a int, CONSTRAINT fk0_detail FOREIGN 
KEY (master_a) REFERENCES master(master_a));
SELECT oid, tgrelid, tgname FROM pg_trigger ;
DELETE FROM pg_trigger WHERE oid = (SELECT min(oid) FROM pg_trigger WHERE 
tgname LIKE 'RI_ConstraintTrigger%' LIMIT 1);
SELECT oid, tgrelid, tgname FROM pg_trigger ;
DROP TABLE detail;
DROP TABLE master;
\c postgres
DROP DATABASE lipa;


The results of the run are as follows.

psql -f test-malicious-dropping-FK-triggers.sql postgres


CREATE DATABASE
You are now connected to database "lipa" as user "postgres".
CREATE TABLE
CREATE TABLE
  oid  | tgrelid |tgname
---+-+--
 39898 |   39889 | RI_ConstraintTrigger_a_39898
 39899 |   39889 | RI_ConstraintTrigger_a_39899
 39900 |   39894 | RI_ConstraintTrigger_c_39900
 39901 |   39894 | RI_ConstraintTrigger_c_39901
(4 rows)

DELETE 1
  oid  | tgrelid |tgname
---+-+--
 39899 |   39889 | RI_ConstraintTrigger_a_39899
 39900 |   39894 | RI_ConstraintTrigger_c_39900
 39901 |   39894 | RI_ConstraintTrigger_c_39901
(3 rows)

psql:test-malicious-dropping-FK-triggers.sql:8: ERROR:  could not find tuple 
for trigger 39898
psql:test-malicious-dropping-FK-triggers.sql:9: ERROR:  could not find tuple 
for trigger 39898
You are now connected to database "postgres" as user "postgres".
DROP DATABASE

> 
> > If this modification is not applied, the detail child table cannot be
> > dropped without prevous dropping the whole FK constraint because 

Re: [HACKERS] Partial index locks

2014-03-22 Thread Thom Brown
On 22 March 2014 05:32, Tom Lane  wrote:
> Thom Brown  writes:
>> Is it necessary for a partial index that doesn't include the row to be
>> involved in locking?
>
> Yes.  You can't determine whether the index needs to get a new entry
> without examining its metadata, and that's what the lock is mainly about.

I see.  Why does this apply to deletes too?

> The only possible alternative would be to take the minimum possible
> lock (AccessShareLock) on each index so its metadata would hold still,
> and then upgrade that to RowExclusiveLock on the one(s) we find need
> insertions.  This is not better; it means *more* lock management traffic
> not less, and lock upgrades increase the potential for deadlocks.

Yes, I can see that wouldn't be an improvement.
-- 
Thom


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


Re: [HACKERS] [RFC] What should we do for reliable WAL archiving?

2014-03-22 Thread Martijn van Oosterhout
On Sat, Mar 22, 2014 at 06:22:37AM +0900, MauMau wrote:
> From: "Jeff Janes" 
> >Do people really just copy the files from one directory of local
> >storage to
> >another directory of local storage?  I don't see the point of that.
> 
> It makes sense to archive WAL to a directory of local storage for
> media recovery.  Here, the local storage is a different disk drive
> which is directly attached to the database server or directly
> connected through SAN.

I'm one of those peope. They are archived into a local directory in
preparation for an rsync over ssh.

> >The recommendation is to refuse to overwrite an existing file of the same
> >name, and exit with failure.  Which essentially brings archiving
> >to a halt,
> >because it keeps trying but it will keep failing.  If we make a custom
> >version, one thing it should do is determine if the existing archived file
> >is just a truncated version of the attempting-to-be archived file, and if
> >so overwrite it.  Because if the first archival command fails with a
> >network glitch, it can leave behind a partial file.
> 
> What I'm trying to address is just an alternative to cp/copy which
> fsyncs a file.  It just overwrites an existing file.

I ran into a related problem with cp, where halfway the copy the disk
was full and I was left with half a WAL file. This caused the rsync to
copy only half a file and the replication broke. This is clearly a
recoverable situation, but it didn't recover in this case.

> Yes, you're right, the failed archive attempt leaves behind a
> partial file which causes subsequent attempts to fail, if you follow
> the PG manual. That's another undesirable point in the current doc.
> To overcome this, someone on this ML recommended me to do "cp %p
> /archive/dir/%f.tmp && mv /archive/dir/%f.tmp /archive/dir/%f".
> Does this solve your problem?

This would probably have handled it, but I find it odd that there's
program to handle restoring of archives properly, but on the archiving
side you have to cobble together your own shell scripts which fail in
various corner cases.

I'd love a program that just Did The Right Thing.

Have a nice day,
-- 
Martijn van Oosterhout  http://svana.org/kleptog/
> He who writes carelessly confesses thereby at the very outset that he does
> not attach much importance to his own thoughts.
   -- Arthur Schopenhauer


signature.asc
Description: Digital signature


Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-22 Thread Piotr Stefaniak


> +myextra = (int *) malloc(sizeof(int));

Please consider not casting malloc(). See 
http://c-faq.com/malloc/mallocnocast.html



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


Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-03-22 Thread Amit Kapila
On Wed, Feb 26, 2014 at 11:56 AM, Yugo Nagata  wrote:
> Thanks for your a lot of comments. I revised the patch according to
> comments from Robert Haas and Marti Raudsepp.

I have started looking into this patch and below are my
initial findings:

1. Dependency is not recorded when to_regclass is used,
however it is recorded when ::regclass is used. Below is
test to demonstrate this point. This change in behaviour is
neither discussed nor mentioned in docs along with patch.

usage of ::regclass

create sequence my_seq;
create table t1 (c1 int, c2 int default 'my_seq'::regclass);
insert into t1 values(1);
insert into t1 values(2);
select * from t1;
 c1 |  c2
+---
  1 | 16390
  2 | 16390
(2 rows)

drop sequence my_seq;
ERROR:  cannot drop sequence my_seq because other
objects depend on it
DETAIL:  default for table t1 column c2 depends on
sequence my_seq
HINT:  Use DROP ... CASCADE to drop the dependent
objects too.

Check pg_depend has entry for dependency of default
value of table-column on seq.

postgres=# select * from pg_depend where refobjid = 16390;
 classid | objid | objsubid | refclassid | refobjid | refobjsubid | deptype
-+---+--++--+-+-
1247 | 16391 |0 |   1259 |16390 |   0 | i
2604 | 16395 |0 |   1259 |16390 |   0 | n
(2 rows)

postgres=# select oid,adrelid from pg_attrdef;
  oid  | adrelid
---+-
 16395 |   16392
(1 row)

usage of to_regclass

create sequence my_seq;
create table t1 (c1 int, c2 int default to_regclass('my_seq'));
insert into t1 values(1);
insert into t1 values(2);
select * from t1;
 c1 |  c2
+---
  1 | 16396
  2 | 16396

select * from pg_depend where refobjid = 16396;
 classid | objid | objsubid | refclassid | refobjid | refobjsubid | deptype
-+---+--++--+-+-
1247 | 16397 |0 |   1259 |16396 |   0 | i
(1 row)

There is only one entry for pg_type which means the
dependent object on sequence is only its type, so it will
allow to drop sequence.

postgres=# drop sequence my_seq;
DROP SEQUENCE

2.
! if (!((Form_pg_type) GETSTRUCT(tup))->typisdefined)
! ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_OBJECT),
! errmsg("type \"%s\" is only a shell",
! TypeNameToString(typeName)),
! parser_errposition(NULL, typeName->location)));

In this case it is not exactly same as object does not exist
so I think we should not avoid error in this case
and infact OID is valid for such a case, but in else part
you have assigned it as InvalidOid which seems to be wrong.

3.
regproc_guts()
! {
! if (!missing_ok)
! ereport(ERROR,
! (errcode(ERRCODE_AMBIGUOUS_FUNCTION),
! errmsg("more than one function named \"%s\"",
! pro_name_or_oid)));
! return false;
! }

In to_regproc(), this patch is trying to supress error other
"object-name-not-found" which as far as I could read the thread
was not the original idea and even the description in docs
says only about "object-name-not-found" case.
Doc Description
+ similar to the regclass, regproc, regoper and regtype casts, except
+ that they return NULL when the object is not found, instead of raising
+ an error.

Even if you want to avoid the other error('s) like
above, then I think it's better to mention the same in docs.

I am bit worried, how is user going to distinguish between
the cases when object-not-found and more-than-one-object.

I think such a problem would not arise if we write a function
for regprocedure rather than for regproc, also manual says regprocedure
is more appropriate for most usecases.
http://www.postgresql.org/docs/devel/static/datatype-oid.html

Also I think one other advantage for doing so is that we
don't need to pass missing_ok paramter to some of the functions
like FuncnameGetCandidates(), OpernameGetCandidates().

I understand that you might be using regproc/regoper or might have
a usecase for it, but is it possible for you to use regprocedure/regoperator
instead of regproc/regoper?


4.
+regclass
+get the table oid

Shouldn't it be better to describe it as  "get the relation oid" as
it can give oid for other objects like index as well.

5.
+   
+ to_regclass, to_regproc, to_regoper and to_regtype are functions
+ similar to the regclass, regproc, regoper and regtype casts, except

In above description, it is better to use 'object identifier types'
rather than 'casts'.

6.
!  * Guts of regoperin and to_regproc.
Here it should be regprocin

7.
* If not found and missing_ok is true, returns false instead of raising
an error.

Above line is more than 80 chars, it should be less than
80 char limit. This needs to be corrected whereever this line is used.

8.
!  * Guts of regtypein and to_regtype.
!  * If the classname is found, returns true and the OID is stored into
*typid_p.

typo in *If the classname is found*, it should be type is found.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterpr

Re: [HACKERS] ALTER TABLE lock strength reduction patch is unsafe Reply-To:

2014-03-22 Thread Simon Riggs
On 21 March 2014 16:11, Simon Riggs  wrote:

>>> + * Be careful to ensure this function is called for Tables and Indexes 
>>> only.
>>> + * It is not currently safe to be called for Views because security_barrier
>>> + * is listed as an option and so would be allowed to be set at a level 
>>> lower
>>> + * than AccessExclusiveLock, which would not be correct.
>>
>> This statement is accepted and takes only ShareUpdateExclusiveLock:
>>
>>   alter table information_schema.triggers set (security_barrier = true);

>> I suggest adding a LOCKMODE field to relopt_gen and adding a
>> reloptions_locklevel() function to determine the required level from an
>> options list.  That puts control of the lock level near the code that
>> understands the implications for each option.  You can then revert the
>> addition of AlterViewInternal() and some of the utility.c changes.
>
> Sure, that's how we code it, but I'm not sure we should introduce that
> feature. The above weirdness is not itself justification.

OK, will follow this path. It's a good idea since it encourages
authors of new "options" to consider properly the lock level that will
be required.

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


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


Re: [HACKERS] ALTER TABLE lock strength reduction patch is unsafe Reply-To:

2014-03-22 Thread Simon Riggs
On 21 March 2014 23:36, Tom Lane  wrote:
> Simon Riggs  writes:
>> On 21 March 2014 20:58, Noah Misch  wrote:
>>> It's not the behavior I would choose for a new product, but I can't see
>>> benefits sufficient to overturn previous decisions to keep it.
>
>> Speechless
>
> The key argument for not "fixing" this is that it would break existing
> pg_dump files.  That's a pretty hard argument to overcome, unfortunately,
> even if you're willing to blow off the possibility that client
> applications might contain similar shortcuts.  We still do our best to
> read dump files from the 7.0 era (see ConvertTriggerToFK() for one example
> of going above and beyond for that); and every so often we do hear of
> people trying to get data out of such ancient servers.  So even if you
> went and fixed pg_dump tomorrow, it'd probably be ten or fifteen years
> before people would let you stop reading dumps from existing versions.

Noah had already convinced me, but thank you for explaining the issue
behind that.

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


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