[HACKERS] proposal: interprocess EXPLAIN PID

[Pavel Stehule]

Hello

I propose a enhancing of EXPLAIN statement about possibility get a plan of
other PostgreSQL process. With some other enhancing this technique can be
interesting for monitoring long duration queries.

Notes, comments?

Regards

Pavel Stehule

Re: [HACKERS] Autonomous Transaction (WIP)

[Rajeev rastogi]

On 09 April 2014 21:25, Robert Haas Wrote:

> >> > Deadlock Detection:
> >> I'm not sure how this would work out internally
> > In order to resolve deadlock, two member variable will be created in
> the structure PROLOCK:
> > Bitmask for lock types currently held by autonomous
> transaction.
> > LOCKMASKholdMaskByAutoTx[MAX_AUTO_TX_LEVEL]
> > Bitmask for lock types currently held by main transaction.
> > LOCKMASKholdMaskByNormalTx
> >
> > Now when we grant the lock to particular transaction, depending on
> > type of transaction, bit Mask will be set for either holdMaskByAutoTx
> or holdMaskByNormalTx.
> > Similar when lock is ungranted, corresponding bitmask will be reset.
> 
> That sounds pretty ugly, not to mention the fact that it will cause a
> substantial increase in the amount of memory required to store
> PROCLOCKs.  It will probably slow things down, too.

Actually I followed above design to keep it align with the existing design. As 
I understand, currently also
all lock conflict is checked based on the corresponding lock bit mask. 

This is good catch that shared memory required will increase but isn't it 
justified from user perspective
since we are allowing more transactions per session and hence memory required 
to store various kind of resources 
will increase.

Since we are just additionally setting the bitmask for each lock (in-case there 
is autonomous transaction, then there will
be one more additional bit mask setting and deadlock check), I don't think it 
should slow down the overall operation. 

Also We can keep number of autonomous transaction configurable(default-0), to 
keep it less impacting incase it is not configured.

An autonomous transaction can also conflict with main transaction, so in order 
to check conflict between them, 
I am distinguishing at this level.

Please correct me If I am wrong anywhere and also please provide your thought 
on this and on overall design.

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 in Windows console and Ctrl-C

[Amit Kapila]

On Fri, Apr 11, 2014 at 3:14 AM, Bruce Momjian  wrote:
>
> Can someone with Windows expertise comment on whether this should be
> applied?

I don't think this is a complete fix, for example what about platform where
_CreateRestrictedToken() is not supported.  For Example, the current
proposed fix will not work for below case:

if (_CreateRestrictedToken == NULL)
{
/*
* NT4 doesn't have CreateRestrictedToken, so just call ordinary
* CreateProcess
*/
write_stderr(_("%s: WARNING: cannot create restricted tokens on this
platform\n"), progname);
if (Advapi32Handle != NULL)
FreeLibrary(Advapi32Handle);
return CreateProcess(NULL, cmd, NULL, NULL, FALSE, 0, NULL, NULL, &si,
processInfo);
}

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

[Kyotaro HORIGUCHI]

# Sorry for accidentialy sending the previous mail unfinished.

## ...and I seem to have bombed uncertain files off out of my
## home directory by accident, too :(

=
Hi, sorry for the absense. I've been back.

Thank you for continuing this discussion.

Attached is the patch following the discussion below.

> >> (2014/04/10 0:08), Tom Lane wrote:
> >>> TBH I think that's barely the tip of the iceberg of cases where this
> >>> patch will get the wrong answer.
> > 
> >>> Also, I don't see it doing anything to check the ordering
> >>> of multiple index columns
> > 
> >> I think that the following code in index_pathkeys_are_extensible() would
> >> check the ordering:
> >> +  if (!pathkeys_contained_in(pathkeys, root->query_pathkeys))
> >> +  return false;
> > 
> > Hm ... if you're relying on that, then what's the point of the new loop
> > at all?
> 
> The point is that from the discussion [1], we allow the index pathkeys
> to be extended to query_pathkeys if each *remaining* pathkey in
> query_pathkey is a Var belonging to the indexed relation.  The code is
> confusing, though.  Sorry, that is my faults.

Hmm, I found that the iterations for the part that already
checked by pathkeys_contained_in are not only useless but a bit
heavy. And the loop seems a little verbose. I did for the patch,
in index_pathkeys_are_extensible,

 - Avoiding duplicate check with pathkeys_contained_in.

   I put similar code to list_nth_cell since it is not exposed
   outside of list.c.

 - Add comment to clarify the purpose of the loop.

 - Simplify the check for the "remaining" keycolumns

I think this makes some things clearer.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index bfb4b9f..ff5c88c 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -1790,6 +1790,7 @@ _outIndexOptInfo(StringInfo str, const IndexOptInfo *node)
 	WRITE_BOOL_FIELD(predOK);
 	WRITE_BOOL_FIELD(unique);
 	WRITE_BOOL_FIELD(immediate);
+	WRITE_BOOL_FIELD(allnotnull);
 	WRITE_BOOL_FIELD(hypothetical);
 	/* we don't bother with fields copied from the pg_am entry */
 }
diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index a912174..4376e95 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -951,8 +951,11 @@ build_index_paths(PlannerInfo *root, RelOptInfo *rel,
 	{
 		index_pathkeys = build_index_pathkeys(root, index,
 			  ForwardScanDirection);
-		useful_pathkeys = truncate_useless_pathkeys(root, rel,
-	index_pathkeys);
+		if (index_pathkeys_are_extensible(root, index, index_pathkeys))
+			useful_pathkeys = root->query_pathkeys;
+		else
+			useful_pathkeys = truncate_useless_pathkeys(root, rel,
+		index_pathkeys);
 		orderbyclauses = NIL;
 		orderbyclausecols = NIL;
 	}
diff --git a/src/backend/optimizer/path/pathkeys.c b/src/backend/optimizer/path/pathkeys.c
index 9179c61..5edca4f 100644
--- a/src/backend/optimizer/path/pathkeys.c
+++ b/src/backend/optimizer/path/pathkeys.c
@@ -502,6 +502,76 @@ build_index_pathkeys(PlannerInfo *root,
 }
 
 /*
+ * index_pathkeys_are_extensible
+ *	  Check whether the pathkeys are extensible to query_pathkeys.
+ */
+bool
+index_pathkeys_are_extensible(PlannerInfo *root,
+			  IndexOptInfo *index,
+			  List *pathkeys)
+{
+	bool		result;
+	ListCell   *lc1, *remain;
+
+	if (root->query_pathkeys == NIL || pathkeys == NIL)
+		return false;
+
+	/* This index is not suitable for pathkey extension */
+	if (!index->unique || !index->immediate || !index->allnotnull)
+		return false;
+
+	/* pathkeys is a prefixing proper subset of index tlist */
+	if (list_length(pathkeys) < list_length(index->indextlist))
+		return false;
+
+	if (!pathkeys_contained_in(pathkeys, root->query_pathkeys))
+		return false;
+
+	if (list_length(pathkeys) == list_length(root->query_pathkeys))
+		return true;
+
+	Assert(list_length(root->query_pathkeys) > list_length(pathkeys));
+
+	/*
+	 * Check if all unchecked elements of query_patheys are simple Vars for
+	 * the same relation.
+	 */
+
+	/* list_nth_cell is not exposed publicly.. */
+	if (list_length(pathkeys) == list_length(root->query_pathkeys) - 1)
+		remain = list_tail(root->query_pathkeys);
+	else
+	{
+		int n = list_length(pathkeys);
+
+		remain = root->query_pathkeys->head;
+		while(n-- > 0) remain = remain->next;
+	}
+
+	result = true;
+	for_each_cell(lc1, remain)
+	{
+		PathKey*pathkey = (PathKey *) lfirst(lc1);
+		ListCell   *lc2;
+		
+		foreach(lc2, pathkey->pk_eclass->ec_members)
+		{
+			EquivalenceMember *member = (EquivalenceMember *) lfirst(lc2);
+			
+			if (!bms_equal(member->em_relids, index->rel->relids) ||
+!IsA(member->em_expr, Var))
+			{
+result = false;
+break;
+			}
+		}
+
+		if (!result) break;
+	}
+	return result;
+}
+
+/*
  * build_expression_pathkey
  *	  Build a pathkeys list that describes an ordering by 

Re: [HACKERS] Get more from indices.

[Kyotaro HORIGUCHI]

Hi, sorry for the absense. I've been back.

Attached is the patch following the discussion below.

> >> (2014/04/10 0:08), Tom Lane wrote:
> >>> TBH I think that's barely the tip of the iceberg of cases where this
> >>> patch will get the wrong answer.
> > 
> >>> Also, I don't see it doing anything to check the ordering
> >>> of multiple index columns
> > 
> >> I think that the following code in index_pathkeys_are_extensible() would
> >> check the ordering:
> >> +  if (!pathkeys_contained_in(pathkeys, root->query_pathkeys))
> >> +  return false;
> > 
> > Hm ... if you're relying on that, then what's the point of the new loop
> > at all?
> 
> The point is that from the discussion [1], we allow the index pathkeys
> to be extended to query_pathkeys if each *remaining* pathkey in
> query_pathkey is a Var belonging to the indexed relation.  The code is
> confusing, though.  Sorry, that is my faults.

Hmm, I found that the iterations for the part that already
checked by pathkeys_contained_in are not only needless but a bit
heavy. And the loop seems a little verbose. I did for the patch,
in index_pathkeys_are_extensible,

 - Avoiding duplicate check with pathkeys_contained_in.

   I put similar code to list_nth_cell since it is not exposed
   outside of list.c.

 - Add comment to clarify the purpose of the loop.

 - Simplify the check for the "remaining" keycolumns




> Thanks,
> 
> [1] http://www.postgresql.org/message-id/29637.1389064...@sss.pgh.pa.us
> 
> 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


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

Re: [HACKERS] [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of < 0

[Amit Kapila]

On Fri, Apr 11, 2014 at 10:00 AM, Amit Kapila  wrote:
> On Thu, Apr 10, 2014 at 5:21 PM, Bruce Momjian  wrote:
>> On Thu, Apr 10, 2014 at 11:05:49AM +0530, Amit Kapila wrote:
>
>> Ah, yes, good point.  This is going to require backpatching then.
>
> I also think so.
>
>>> I think it's better to use check like below, just for matter of
>>> consistency with other place
>>> if (sock == INVALID_SOCKET)
>>
>> Agreed.  That is how I have coded the patch.
>
> Sorry, I didn't checked the latest patch before that comment.
>
> I verified that your last patch is fine.  Regression test also went fine.

I have noticed small thing which I forgot to mention in previous mail.
I think below added extra line is not required.

  int
  PQsocket(const PGconn *conn)
  {
+

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] [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of < 0

[Amit Kapila]

On Thu, Apr 10, 2014 at 5:21 PM, Bruce Momjian  wrote:
> On Thu, Apr 10, 2014 at 11:05:49AM +0530, Amit Kapila wrote:

> Ah, yes, good point.  This is going to require backpatching then.

I also think so.

>> I think it's better to use check like below, just for matter of
>> consistency with other place
>> if (sock == INVALID_SOCKET)
>
> Agreed.  That is how I have coded the patch.

Sorry, I didn't checked the latest patch before that comment.

I verified that your last patch is fine.  Regression test also went fine.

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] Adding unsigned 256 bit integers

[Leon Smith]

pgmp is also worth mentioning here,   and it's likely to be more efficient
than the numeric type or something you hack up yourself:

http://pgmp.projects.pgfoundry.org/

Best,
Leon


On Thu, Apr 10, 2014 at 10:11 AM, k...@rice.edu  wrote:

> On Thu, Apr 10, 2014 at 09:13:47PM +0800, Olivier Lalonde wrote:
> > I was wondering if there would be any way to do the following in
> PostgreSQL:
> >
> > UPDATE cryptotable SET work = work + 'some big hexadecimal number'
> >
> > where work is an unsigned 256 bit integer. Right now my column is a
> > character varying(64) column (hexadecimal representation of the number)
> but
> > I would be happy to switch to another data type if it lets me do the
> > operation above.
> >
> > If it's not possible with vanilla PostgreSQL, are there extensions that
> > could help me?
> >
> > --
> > - Oli
> >
> > Olivier Lalonde
> > http://www.syskall.com <-- connect with me!
> >
>
> Hi Olivier,
>
> Here are some sample pl/pgsql helper functions that I have written for
> other purposes. They use integers but can be adapted to use numeric.
>
> Regards,
> Ken
> ---
> CREATE OR REPLACE FUNCTION hex2dec(t text) RETURNS integer AS $$
> DECLARE
>   r RECORD;
> BEGIN
>   FOR r IN EXECUTE 'SELECT x'''||t||'''::integer AS hex' LOOP
> RETURN r.hex;
>   END LOOP;
> END
> $$ LANGUAGE plpgsql IMMUTABLE STRICT;
> ---
>
> ---
> CREATE OR REPLACE FUNCTION bytea2int (
>   in_string BYTEA
> ) RETURNS INTEGER AS $$
>
> DECLARE
>
>   b1 INTEGER := 0;
>   b2 INTEGER := 0;
>   b3 INTEGER := 0;
>   b4 INTEGER := 0;
>   out_int INTEGER := 0;
>
> BEGIN
>
>   CASE OCTET_LENGTH(in_string)
> WHEN 1 THEN
>   b4 := get_byte(in_string, 0);
> WHEN 2 THEN
>   b3 := get_byte(in_string, 0);
>   b4 := get_byte(in_string, 1);
> WHEN 3 THEN
>   b2 := get_byte(in_string, 0);
>   b3 := get_byte(in_string, 1);
>   b4 := get_byte(in_string, 2);
> WHEN 4 THEN
>   b1 := get_byte(in_string, 0);
>   b2 := get_byte(in_string, 1);
>   b3 := get_byte(in_string, 2);
>   b4 := get_byte(in_string, 3);
>   END CASE;
>
>   out_int := (b1 << 24) + (b2 << 16) + (b3 << 8) + b4;
>
>   RETURN(out_int);
> END;
> $$ LANGUAGE plpgsql IMMUTABLE;
> ---
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Re: [HACKERS] WIP patch (v2) for updatable security barrier views

[Stephen Frost]

Dean, Craig, all,

* Dean Rasheed (dean.a.rash...@gmail.com) wrote:
> This is reflected in the change to the regression test output where,
> in one of the tests, the ctids for the table to update are no longer
> coming from the same table. I think a better approach is to push down
> the rowmark into the subquery so that any locking required applies to
> the pushed down RTE --- see the attached patch.

I'm working through this patch and came across a few places where I
wanted to ask questions (as much for my own edification as questioning
the actual implementation).  Also, feel free to point out if I'm
bringing up something which has already been discussed.  I've been
trying to follow the discussion but it's been a while and my memory may
have faded.

While in the planner, we need to address the case of a child RTE which
has been transformed from a relation to a subquery.  That all makes
perfect sense, but I'm wondering if it'd be better to change this
conditional:

!   if (rte1->rtekind == RTE_RELATION &&
!   rte1->securityQuals != NIL &&
!   rte2->rtekind == RTE_SUBQUERY)

which essentially says "if a relation was changed to a subquery *and*
it has security quals then we need to update the entry" into one like
this:

!   if (rte1->rtekind == RTE_RELATION &&
!   rte2->rtekind == RTE_SUBQUERY)
!   { 
!   Assert(rte1->securityQuals != NIL);
!   ...

which changes it to "if a relation was changed to a subquery, it had
better be because it's got securityQuals that we're dealing with".  My
general thinking here is that we'd be better off with the Assert()
firing during some later development which changes things in this area
than skipping the change because there aren't any securityQuals and then
expecting everything to be fine with the subquery actually being a
relation..

I could see flipping that around too, to check if there are
securityQuals and then Assert() on the change from relation to subquery-
after all, if there are securityQuals then it *must* be a subquery,
right?

A similar case exists in prepunion.c where we're checking if we should
recurse while in adjust_appendrel_attrs_mutator()- the check exists as

!   if (IsA(node, Query))

(... which used to be an Assert(!IsA(node, Query)) ...)

but the comment is then quite clear that we should only be doing this in
the security-barrier case; perhaps we should Assert() there to that
effect?  It'd certainly make me feel a bit better about removing the two
Asserts() which were there; presumably we had to also remove the
Assert(!IsA(node, SubLink)) ?

Also, it seems like there should be a check_stack_depth() call here now?

That covers more-or-less everything outside of prepsecurity.c itself.
I'm planning to review that tomorrow night.  In general, I'm pretty
happy with the shape of this.  The wiki and earlier discussions were
quite useful.  My one complaint about this is that it feels like a few
more comments and more documentation updates would be warrented; and in
particular we need to make note of the locking "gotcha" in the docs.
That's not a "solution", of course, but since we know about it we should
probably make sure users are aware.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Get more from indices.

[Etsuro Fujita]

(2014/04/10 22:25), Tom Lane wrote:
> Etsuro Fujita  writes:
>> (2014/04/10 0:08), Tom Lane wrote:
>>> TBH I think that's barely the tip of the iceberg of cases where this
>>> patch will get the wrong answer.
> 
>>> Also, I don't see it doing anything to check the ordering
>>> of multiple index columns
> 
>> I think that the following code in index_pathkeys_are_extensible() would
>> check the ordering:
>> +if (!pathkeys_contained_in(pathkeys, root->query_pathkeys))
>> +return false;
> 
> Hm ... if you're relying on that, then what's the point of the new loop
> at all?

The point is that from the discussion [1], we allow the index pathkeys
to be extended to query_pathkeys if each *remaining* pathkey in
query_pathkey is a Var belonging to the indexed relation.  The code is
confusing, though.  Sorry, that is my faults.

Thanks,

[1] http://www.postgresql.org/message-id/29637.1389064...@sss.pgh.pa.us

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] PostgreSQL in Windows console and Ctrl-C

[Haribabu Kommi]

On Fri, Apr 11, 2014 at 12:12 PM, Bruce Momjian  wrote:
> On Fri, Apr 11, 2014 at 11:58:58AM +1000, Haribabu Kommi wrote:
>> On Fri, Apr 11, 2014 at 7:44 AM, Bruce Momjian  wrote:
>> >
>> > Can someone with Windows expertise comment on whether this should be
>> > applied?
>>
>> I tested the same in windows and it is working as specified.
>> The same background running server can be closed with ctrl+break command.
>
> OK.  If I apply this, are you able to test to see if the problem is
> fixed?

I already tested in HEAD by applying the attached patch in the earlier mail.
with ctrl+c command the background process is not closed.
But with ctrl+break command the same can be closed.
if this behavior is fine, then we can apply patch.

Regards,
Hari Babu
Fujitsu Australia


-- 
Sent 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 in Windows console and Ctrl-C

[Bruce Momjian]

On Fri, Apr 11, 2014 at 11:58:58AM +1000, Haribabu Kommi wrote:
> On Fri, Apr 11, 2014 at 7:44 AM, Bruce Momjian  wrote:
> >
> > Can someone with Windows expertise comment on whether this should be
> > applied?
> 
> I tested the same in windows and it is working as specified.
> The same background running server can be closed with ctrl+break command.

OK.  If I apply this, are you able to test to see if the problem is
fixed?

-- 
  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] PostgreSQL in Windows console and Ctrl-C

[Haribabu Kommi]

On Fri, Apr 11, 2014 at 7:44 AM, Bruce Momjian  wrote:
>
> Can someone with Windows expertise comment on whether this should be
> applied?

I tested the same in windows and it is working as specified.
The same background running server can be closed with ctrl+break command.

> ---
>
> On Tue, Jan  7, 2014 at 12:44:33PM +0100, Christian Ullrich wrote:
>> Hello all,
>>
>> when pg_ctl start is used to run PostgreSQL in a console window on
>> Windows, it runs in the background (it is terminated by closing the
>> window, but that is probably inevitable). There is one problem,
>> however: The first Ctrl-C in that window, no matter in which
>> situation, will cause the background postmaster to exit. If you,
>> say, ping something, and press Ctrl-C to stop ping, you probably
>> don't want the database to go away, too.
>>
>> The reason is that Windows delivers the Ctrl-C event to all
>> processes using that console, not just to the foreground one.
>>
>> Here's a patch to fix that. "pg_ctl stop" still works, and it has no
>> effect when running as a service, so it should be safe. It starts
>> the postmaster in a new process group (similar to calling setpgrp()
>> after fork()) that does not receive Ctrl-C events from the console
>> window.
>>
>> --
>> Christian
>
>> diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
>> new file mode 100644
>> index 50d4586..89a9544
>> *** a/src/bin/pg_ctl/pg_ctl.c
>> --- b/src/bin/pg_ctl/pg_ctl.c
>> *** CreateRestrictedProcess(char *cmd, PROCE
>> *** 1561,1566 
>> --- 1561,1567 
>>   HANDLE  restrictedToken;
>>   SID_IDENTIFIER_AUTHORITY NtAuthority = {SECURITY_NT_AUTHORITY};
>>   SID_AND_ATTRIBUTES dropSids[2];
>> + DWORD   flags;
>>
>>   /* Functions loaded dynamically */
>>   __CreateRestrictedToken _CreateRestrictedToken = NULL;
>> *** CreateRestrictedProcess(char *cmd, PROCE
>> *** 1636,1642 
>>   AddUserToTokenDacl(restrictedToken);
>>   #endif
>>
>> ! r = CreateProcessAsUser(restrictedToken, NULL, cmd, NULL, NULL, TRUE, 
>> CREATE_SUSPENDED, NULL, NULL, &si, processInfo);
>>
>>   Kernel32Handle = LoadLibrary("KERNEL32.DLL");
>>   if (Kernel32Handle != NULL)
>> --- 1637,1650 
>>   AddUserToTokenDacl(restrictedToken);
>>   #endif
>>
>> ! flags = CREATE_SUSPENDED;
>> !
>> ! /* Protect console process from Ctrl-C */
>> ! if (!as_service) {
>> ! flags |= CREATE_NEW_PROCESS_GROUP;
>> ! }
>> !
>> ! r = CreateProcessAsUser(restrictedToken, NULL, cmd, NULL, NULL, TRUE, 
>> flags, NULL, NULL, &si, processInfo);
>>
>>   Kernel32Handle = LoadLibrary("KERNEL32.DLL");
>>   if (Kernel32Handle != NULL)

Regards,
Hari Babu
Fujitsu Australia


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

Re: [HACKERS] WAL replay bugs

[Sachin D. Kotwal]

On Thu, Apr 10, 2014 at 6:21 PM, Heikki Linnakangas  wrote:

> On 04/10/2014 10:52 AM, sachin kotwal wrote:
>
>>
>> I executed given  steps many times to produce this bug.
>> But still I unable to hit this bug.
>> I used attached scripts to produce this bug.
>>
>> Can I get scripts to produce this bug?
>>
>>
>>
> Oh, I can't reproduce it using that script either. I must've used some
> variation of it, and posted wrong script.
>
> The attached seems to do the trick. I changed the INSERT statements
> slightly, so that all the new rows have the same key.
>
> Thanks for verifying this!
>

Thanks to explain the case to produce this bug.
I am able to produce this bug by using latest scripts from last mail.
I applied patch submitted for this bug and re-run the scripts.
Now it is giving correct result.


Thanks and Regards,

Sachin Kotwal

Re: [HACKERS] [PATCH] Negative Transition Aggregate Functions (WIP)

[Florian Pflug]

On Apr11, 2014, at 01:30 , Tom Lane  wrote:
> Florian Pflug  writes:
>> As for evidence - have you looked at the patch I posted? I'd be very
>> interested to know if it removes the performance differences you saw.
> 
> (1) You can't really prove the absence of a performance issue by showing
> that one specific aggregate doesn't have an issue.

I'm claiming that SUM(int4) is about as simple as it gets, so if the
effect can be mitigated there, it can be mitigated everywhere. The more
complex a forward-only transition function is, the less will and added if
or two hurt.

> (2) These results
> (mine as well as yours) seem mighty platform-dependent, so the fact you
> don't see an issue doesn't mean someone else won't.

Yeah, though *any* change - mine, the one your propose, and any other -
has the potential to hurt some platform due to weird interactions (say,
cache trashing). 

> (3) A new
> FunctionCallInfoData field just for this?  Surely not.  There's got to be
> a distributed cost to that (although I notice you didn't bother
> initializing the field most places, which is cheating).

I think the field doesn't actually increase the size of the structure at
all - at least not if the bool before it has size 1 and the short following
it is 2-byte aligned. Or at least that was why I made it a char, and added
it at the place I did. But I might be missing something...

Also, InitFunctionCallInfoData *does* initialize the flags to zero. Though
maybe not everybody uses that - I didn't check, this was just a quick hack.

> Now point 3 could be addressed by doing the signaling in some other way
> with the existing context field.  But it's still the case that you're
> trying to band-aid a bad design.  There's no good reason to make the sfunc
> do extra work to be invertible in contexts where we know, with certainty,
> that that work is useless.

This is the principal point where we disagree, I think. From my POV, the
problem isn't invertibility here at all. Heck, SUM(int4) wouldn't need
*any* extra state at all to be invertible, if it weren't for those pesky
issues surrounding NULL handling. In fact, an earlier version of the
invtrans patch *did* use int4_sum as SUM(int4)'s transfer functions! The
only reason this doesn't work nowadays is that Dean didn't like the
forward-nonstrict-but-inverse-strict special case that made this work.

The way I see it, the main problem is the drop in performance that comes
from using a pass-by-ref state type. Which IMHO happens because this
*already* is a heavily band-aided design - all the state validation and
"if (AggCheckCallContext(,NULL))" stuff really works around the fact that
transition functions *aren't* supposed to be user-called, yet they are,
and must deal.

Which is why I feel that having two separate sets of transition functions
and state types solves the wrong problem. If we find a way to prevent
transition functions from being called directly, we'll shave a few cycles
of quite a few existing aggregates, invertible or not. If we find a way
around the initfunc-for-internal-statetype problem you discovered, the
implementation would get much simpler, since we could then make nearly
all of them strict. And this would again shave off a few cycles - for lots
of NULL inputs, the effect could even be large.

Compared to that, the benefit of having a completely separate set of
transition functions and state types for the invertible case is much
less tangible, IMHO.

> Especially not when we know that even a few instructions of extra work
> can be performance-significant.

But where do we draw that line? nodeWindowAgg.c quite certainly wastes
about as many cycles as int4_avg_accum does on various checks that are
unnecessary unless in the non-sliding window case. Do we duplicate those
functions too?

best regards,
Florian Pflug





-- 
Sent 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 \d+ and oid display

[Bruce Momjian]

On Thu, Apr 10, 2014 at 07:58:55PM -0400, Tom Lane wrote:
> Bruce Momjian  writes:
> > It also has changed the OID status to only display if it exists.  One
> > question that came up with Robert is whether OID status should appear
> > for \d as well, now that is only shows up when present.
> 
> Yeah, I was wondering about that too.  If part of the argument here is
> to make these two displays act more alike, it seems inconsistent that
> one is emitted by \d while the other only comes out with \d+.
> 
> Of course, there are two ways to fix that: maybe the replica info
> also only belongs in \d+?

OK, I changed my patch to only show replica info for \d+.  If we decide
to change them to both display for \d, I will update it again.

-- 
  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] psql \d+ and oid display

[Tom Lane]

Bruce Momjian  writes:
> It also has changed the OID status to only display if it exists.  One
> question that came up with Robert is whether OID status should appear
> for \d as well, now that is only shows up when present.

Yeah, I was wondering about that too.  If part of the argument here is
to make these two displays act more alike, it seems inconsistent that
one is emitted by \d while the other only comes out with \d+.

Of course, there are two ways to fix that: maybe the replica info
also only belongs in \d+?

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] psql \d+ and oid display

[Bruce Momjian]

On Thu, Apr 10, 2014 at 01:10:35PM -0400, Bruce Momjian wrote:
> On Thu, Apr 10, 2014 at 01:05:32PM -0400, Greg Stark wrote:
> > If it's conditional I think "when it matches a guc" is too hard for users to
> > use.
> 
> Yes, we gave up on having the OID display match the GUC;  we just
> display something if and only if it oids are present.
> 
> Robert is talking about the "Identity Replica" display, which is new in
> 9.4 and should match how we display oids.  Right now "Identity Replica"
> only displays something if the user changed the default.
> 
> > I think "say nothing if oids are off and say something of their on" would be
> > fine. It would result in clutter for users which oids on by default but 
> > that's
> > a non default setting.
> 
> Yes, that is what the proposed patch does.

I talked to Robert via voice to understand his concerns.  He clearly
explained the complexity of how "Replica Identity" is set.  I, of
course, am concerned about user confusion in showing these values.

What I did was develop the attached patch which clarifies the default
for system and non-system tables, documents when psql displays it, and
improves the display for pg_catalog tables that aren't equal to NONE.

It also has changed the OID status to only display if it exists.  One
question that came up with Robert is whether OID status should appear
for \d as well, now that is only shows up when present.

I am hopeful this patch is closer to general agreement.

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

  + Everyone has their own god. +
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
new file mode 100644
index 0b08f83..ac6a4a4
*** a/doc/src/sgml/ref/alter_table.sgml
--- b/doc/src/sgml/ref/alter_table.sgml
*** ALTER TABLE [ IF EXISTS ] 
This form changes the information which is written to the write-ahead log
to identify rows which are updated or deleted.  This option has no effect
!   except when logical replication is in use.  DEFAULT records the 
old values of the columns of the primary key, if any.  USING INDEX
records the old values of the columns covered by the named index, which
must be unique, not partial, not deferrable, and include only columns marked
NOT NULL.  FULL records the old values of all columns
in the row.  NOTHING records no information about the old row.
In all cases, no old values are logged unless at least one of the columns
that would be logged differs between the old and new versions of the row.
   
--- 608,621 
   
This form changes the information which is written to the write-ahead log
to identify rows which are updated or deleted.  This option has no effect
!   except when logical replication is in use.  DEFAULT
!   (the default for non-system tables) records the 
old values of the columns of the primary key, if any.  USING INDEX
records the old values of the columns covered by the named index, which
must be unique, not partial, not deferrable, and include only columns marked
NOT NULL.  FULL records the old values of all columns
in the row.  NOTHING records no information about the old row.
+   (This is the default for system tables.)
In all cases, no old values are logged unless at least one of the columns
that would be logged differs between the old and new versions of the row.
   
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
new file mode 100644
index 85899d7..0b91d45
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
*** testdb=>
*** 951,957 
  The command form \d+ is identical, except that
  more information is displayed: any comments associated with the
  columns of the table are shown, as is the presence of OIDs in the
! table, the view definition if the relation is a view.
  
  
  
--- 951,959 
  The command form \d+ is identical, except that
  more information is displayed: any comments associated with the
  columns of the table are shown, as is the presence of OIDs in the
! table, the view definition if the relation is a view, a non-default 
! replica
! identity setting.
  
  
  
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
new file mode 100644
index d1447fe..0ff7950
*** a/src/bin/psql/describe.c
--- b/src/bin/psql/describe.c
*** describeOneTableDetails(const char *sche
*** 2350,2357 
  			 * No need to display default values;  we already display a
  			 * REPLICA IDENTITY marker on indexes.
  			 */
! 			tableinfo.relreplident != 'd' && tableinfo.relreplident != 'i' &&
! 			strcmp(schemaname, "pg_catalog") != 0)
  		{
  			const char *s = _("Replica Identity");
  
--- 2350,2358

Re: [HACKERS] [PATCH] Negative Transition Aggregate Functions (WIP)

[Tom Lane]

Florian Pflug  writes:
> My argument is that is costs us more complexity to duplicate everything
> for the invertible case, *and* the result seems less flexible - not
> from the POV of aggregate implementations, but from the POV of future
> extensions.

[ shrug... ]  You can argue against any feature whatsoever by claiming
that it might possibly conflict with something we would wish to do
sometime in the future.  I think you need to have a much more concrete
argument about specific issues this will cause in order to be convincing.
For all we know about ROLLUP/CUBE implementation issues right now, doing
this feature with separate implementations might make that easier not
harder.  (I note that the crux of my complaint right now is that we're
asking sfuncs to serve two masters --- how's it going to be better when
they have to serve three or four?)

> As for evidence - have you looked at the patch I posted? I'd be very
> interested to know if it removes the performance differences you saw.

(1) You can't really prove the absence of a performance issue by showing
that one specific aggregate doesn't have an issue.  (2) These results
(mine as well as yours) seem mighty platform-dependent, so the fact you
don't see an issue doesn't mean someone else won't.  (3) A new
FunctionCallInfoData field just for this?  Surely not.  There's got to be
a distributed cost to that (although I notice you didn't bother
initializing the field most places, which is cheating).

Now point 3 could be addressed by doing the signaling in some other way
with the existing context field.  But it's still the case that you're
trying to band-aid a bad design.  There's no good reason to make the sfunc
do extra work to be invertible in contexts where we know, with certainty,
that that work is useless.  Especially not when we know that even a few
instructions of extra work can be performance-significant.

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] psql \d+ and oid display

[Bruce Momjian]

On Tue, Apr  1, 2014 at 10:45:29AM -0700, Jeff Janes wrote:
> I am suggesting it for at least some other things.  I'm rather aggrieved that 
> "
> \d+" without argument shows you the size and the description/comment for every
> table, but "\d+ foo" does not show you the size and description/comment of the
> specific table you just asked for.
> 
> Not so aggrieved that I wrote and submitted a patch, you might notice; but 
> I'll
> get to it eventually if no one beats me to it.

Agree, that is kind of odd.  Should that be a TODO?

-- 
  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] Negative Transition Aggregate Functions (WIP)

[Florian Pflug]

On Apr11, 2014, at 00:07 , Tom Lane  wrote:
> Florian Pflug  writes:
>> I still think you're getting ahead of yourselves here. The number of
>> aggregates which benefit from this is tiny SUM(int2,int4) and maybe
>> BOOL_{AND,OR}. And in the SUM(int2,int4) case *only* on 64-bit archs -
>> for the others, the state type is already pass-by-ref.
> 
> That argument is reasonable for the initfunc idea, but it doesn't apply
> otherwise.

Why not? AFAICS, the increase in cost comes from going from an
by-value to a by-reference state type. Once you're using a by-refence
type, you already pay the overhead of the additional dereferences, and
for calling AggCheckCallContext() or some equivalent. 

>> Another reason I'm so opposed to this is that inverse transition
>> functions might not be the last kind of transition functions we ever
>> add. For example, if we ever get ROLLUP/CUBE, we might want to have
>> a mergefunc which takes two aggregation states and combines them 
>> into one. What do we do if we add those?
> 
> Make more pg_aggregate columns.  What exactly do you think is either
> easier or harder about such cases?  Also, maybe I'm misremembering
> the spec, but ROLLUP/CUBE wouldn't apply to window functions would
> they?  So if your argument is based on the assumption that these
> features need to combine, I'm not sure it's true.

Well, it was just an example - there might be other future extensions
which *do* need to combine. And we've been known to go beyond the spec
sometimes...

> Furthermore, I do not buy the argument that if we hack hard enough,
> we can make the performance cost of forcing the sfunc to do double duty
> negligible.  In the first place, that argument is unsupported by much
> evidence, and in the second place, it will certainly cost us complexity
> to make the performance issue go away.  Instead we can just design the
> problem out, for nothing that I see as a serious drawback.

My argument is that is costs us more complexity to duplicate everything
for the invertible case, *and* the result seems less flexible - not
from the POV of aggregate implementations, but from the POV of future
extensions.

As for evidence - have you looked at the patch I posted? I'd be very
interested to know if it removes the performance differences you saw.

best regards,
Florian Pflug



-- 
Sent 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] Negative Transition Aggregate Functions (WIP)

[Tom Lane]

Florian Pflug  writes:
> I still think you're getting ahead of yourselves here. The number of
> aggregates which benefit from this is tiny SUM(int2,int4) and maybe
> BOOL_{AND,OR}. And in the SUM(int2,int4) case *only* on 64-bit archs -
> for the others, the state type is already pass-by-ref.

That argument is reasonable for the initfunc idea, but it doesn't apply
otherwise.

> Another reason I'm so opposed to this is that inverse transition
> functions might not be the last kind of transition functions we ever
> add. For example, if we ever get ROLLUP/CUBE, we might want to have
> a mergefunc which takes two aggregation states and combines them 
> into one. What do we do if we add those?

Make more pg_aggregate columns.  What exactly do you think is either
easier or harder about such cases?  Also, maybe I'm misremembering
the spec, but ROLLUP/CUBE wouldn't apply to window functions would
they?  So if your argument is based on the assumption that these
features need to combine, I'm not sure it's true.

The bottom line for me is that it seems conceptually far cleaner to
make the moving-aggregate support be independent than to insist that
it share an stype and sfunc with the plain case.

Furthermore, I do not buy the argument that if we hack hard enough,
we can make the performance cost of forcing the sfunc to do double duty
negligible.  In the first place, that argument is unsupported by much
evidence, and in the second place, it will certainly cost us complexity
to make the performance issue go away.  Instead we can just design the
problem out, for nothing that I see as a serious drawback.

regards, tom lane


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

Re: [HACKERS] [PATCH] Negative Transition Aggregate Functions (WIP)

[Florian Pflug]

On Apr10, 2014, at 21:34 , Dean Rasheed  wrote:
> On 10 April 2014 19:54, Tom Lane  wrote:
>> So if we go with that terminology, perhaps these names for the
>> new CREATE AGGREGATE parameters:
>> 
>> initfuncapplies to plain aggregation, mutually exclusive with 
>> initcond
>> msfunc  (or just mfunc?) forward transition for moving-agg mode
>> mifunc  inverse transition for moving-agg mode
>> mstype  state datatype for moving-agg mode
>> msspace space estimate for mstype
>> mfinalfunc  final function for moving-agg mode
>> minitfunc   "firsttrans" for moving-agg mode
>> minitcond   mutually exclusive with minitfunc
> 
> I think I prefer "mfunc" to "msfunc", but perhaps that's just my
> natural aversion to the "ms" prefix :-)
> 
> Also, perhaps "minvfunc" rather than "mifunc" because "i" by itself
> could mean "initial".

I still think you're getting ahead of yourselves here. The number of
aggregates which benefit from this is tiny SUM(int2,int4) and maybe
BOOL_{AND,OR}. And in the SUM(int2,int4) case *only* on 64-bit archs -
for the others, the state type is already pass-by-ref.

I don't think we should be additional that much additional machinery
until it has been conclusively demonstrated that the performance
regression cannot be fixed any other way. Which, quite frankly, I
don't believe. Nothing in int4_avg_accum looks particularly expensive,
and the things that *do* seem to cost something certainly can be made
cheaper. c.f. the patch I just posted.

Another reason I'm so opposed to this is that inverse transition
functions might not be the last kind of transition functions we ever
add. For example, if we ever get ROLLUP/CUBE, we might want to have
a mergefunc which takes two aggregation states and combines them 
into one. What do we do if we add those? Add yet a another set of
"mergable" transition functions? What about the various combinations
of invertible/non-invertible mergable/non-mergable that could result?
The opportunity cost seems pretty high here...

best regards,
Florian Pflug



-- 
Sent 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] Negative Transition Aggregate Functions (WIP)

[Tom Lane]

Dean Rasheed  writes:
> I was imagining that firsttrans would only be passed the first value
> to be aggregated, not any previous state, and that it would be illegal
> to specify both an initcond and a firsttrans function.

> The forward transition function would only be called for values after
> the first, by which point the state would be non-null, and so it could
> be made strict in most cases. The same would apply to the invertible
> transition functions, so they wouldn't have to do null counting, which
> in turn would make their state types simpler.

I put together a very fast proof-of-concept patch for this (attached).
It has a valid execution path for an aggregate with initfunc, but I didn't
bother writing the CREATE AGGREGATE support yet.  I made sum(int4) work
as you suggest, marking the transfn strict and ripping out int4_sum's
internal support for null inputs.  The result seems to be about a 4% or so
improvement in the overall aggregation speed, for a simple "SELECT
sum(int4col) FROM table" query.  So from a performance standpoint this
seems only marginally worth doing.  The real problem is not that 4% isn't
worth the trouble, it's that AFAICS the only built-in aggregates that
can benefit are sum(int2) and sum(int4).  So that looks like a rather
narrow use-case.

You had suggested upthread that we could use this idea to make the
transition functions strict for aggregates using "internal" transition
datatypes, but that does not work because the initfunc would violate
the safety rule that a function returning internal must take at least
one internal-type argument.  That puts a pretty strong damper on the
usefulness of the approach, given how many internal-transtype aggregates
we have (and the moving-aggregate case is not going to be better is it?)

So at this point I'm feeling unexcited about the initfunc idea.
Unless it does something really good for the moving-aggregate case,
I think we should drop it.

regards, tom lane

diff --git a/src/backend/catalog/pg_aggregate.c b/src/backend/catalog/pg_aggregate.c
index fe6dc8a..1ca5c8f 100644
*** a/src/backend/catalog/pg_aggregate.c
--- b/src/backend/catalog/pg_aggregate.c
*** AggregateCreate(const char *aggName,
*** 390,395 
--- 390,396 
  	values[Anum_pg_aggregate_aggfnoid - 1] = ObjectIdGetDatum(procOid);
  	values[Anum_pg_aggregate_aggkind - 1] = CharGetDatum(aggKind);
  	values[Anum_pg_aggregate_aggnumdirectargs - 1] = Int16GetDatum(numDirectArgs);
+ 	values[Anum_pg_aggregate_agginitfn - 1] = ObjectIdGetDatum(InvalidOid);
  	values[Anum_pg_aggregate_aggtransfn - 1] = ObjectIdGetDatum(transfn);
  	values[Anum_pg_aggregate_aggfinalfn - 1] = ObjectIdGetDatum(finalfn);
  	values[Anum_pg_aggregate_aggsortop - 1] = ObjectIdGetDatum(sortop);
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 7e4bca5..2671c49 100644
*** a/src/backend/executor/nodeAgg.c
--- b/src/backend/executor/nodeAgg.c
*** typedef struct AggStatePerAggData
*** 152,157 
--- 152,158 
  	int			numTransInputs;
  
  	/* Oids of transfer functions */
+ 	Oid			initfn_oid;		/* may be InvalidOid */
  	Oid			transfn_oid;
  	Oid			finalfn_oid;	/* may be InvalidOid */
  
*** typedef struct AggStatePerAggData
*** 160,165 
--- 161,167 
  	 * corresponding oid is not InvalidOid.  Note in particular that fn_strict
  	 * flags are kept here.
  	 */
+ 	FmgrInfo	initfn;
  	FmgrInfo	transfn;
  	FmgrInfo	finalfn;
  
*** typedef struct AggHashEntryData
*** 296,301 
--- 298,306 
  static void initialize_aggregates(AggState *aggstate,
  	  AggStatePerAgg peragg,
  	  AggStatePerGroup pergroup);
+ static void initialize_transition_value(AggState *aggstate,
+ 			AggStatePerAgg peraggstate,
+ 			AggStatePerGroup pergroupstate);
  static void advance_transition_function(AggState *aggstate,
  			AggStatePerAgg peraggstate,
  			AggStatePerGroup pergroupstate);
*** initialize_aggregates(AggState *aggstate
*** 403,408 
--- 408,498 
  }
  
  /*
+  * Initialize the transition value upon reaching the first non-NULL input(s).
+  *
+  * We use this code when the initcond is NULL and the transfn is strict,
+  * which otherwise would mean the transition value can never become non-null.
+  * If an initfn has been provided, call it on the current input value(s);
+  * otherwise, take the current input value as the new transition value.
+  * (In the latter case, we already checked that this is okay datatype-wise.)
+  */
+ static void
+ initialize_transition_value(AggState *aggstate,
+ 			AggStatePerAgg peraggstate,
+ 			AggStatePerGroup pergroupstate)
+ {
+ 	FunctionCallInfo tfcinfo = &peraggstate->transfn_fcinfo;
+ 	MemoryContext oldContext;
+ 
+ 	if (OidIsValid(peraggstate->initfn_oid))
+ 	{
+ 		FunctionCallInfoData fcinfo;
+ 		int			numInitArgs;
+ 		Datum		newVal;
+ 
+ 		/* We run the transition functions in per-input-tuple memory context *

Re: [HACKERS] PostgreSQL in Windows console and Ctrl-C

[Bruce Momjian]

Can someone with Windows expertise comment on whether this should be
applied?

---

On Tue, Jan  7, 2014 at 12:44:33PM +0100, Christian Ullrich wrote:
> Hello all,
> 
> when pg_ctl start is used to run PostgreSQL in a console window on
> Windows, it runs in the background (it is terminated by closing the
> window, but that is probably inevitable). There is one problem,
> however: The first Ctrl-C in that window, no matter in which
> situation, will cause the background postmaster to exit. If you,
> say, ping something, and press Ctrl-C to stop ping, you probably
> don't want the database to go away, too.
> 
> The reason is that Windows delivers the Ctrl-C event to all
> processes using that console, not just to the foreground one.
> 
> Here's a patch to fix that. "pg_ctl stop" still works, and it has no
> effect when running as a service, so it should be safe. It starts
> the postmaster in a new process group (similar to calling setpgrp()
> after fork()) that does not receive Ctrl-C events from the console
> window.
> 
> -- 
> Christian

> diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
> new file mode 100644
> index 50d4586..89a9544
> *** a/src/bin/pg_ctl/pg_ctl.c
> --- b/src/bin/pg_ctl/pg_ctl.c
> *** CreateRestrictedProcess(char *cmd, PROCE
> *** 1561,1566 
> --- 1561,1567 
>   HANDLE  restrictedToken;
>   SID_IDENTIFIER_AUTHORITY NtAuthority = {SECURITY_NT_AUTHORITY};
>   SID_AND_ATTRIBUTES dropSids[2];
> + DWORD   flags;
>   
>   /* Functions loaded dynamically */
>   __CreateRestrictedToken _CreateRestrictedToken = NULL;
> *** CreateRestrictedProcess(char *cmd, PROCE
> *** 1636,1642 
>   AddUserToTokenDacl(restrictedToken);
>   #endif
>   
> ! r = CreateProcessAsUser(restrictedToken, NULL, cmd, NULL, NULL, TRUE, 
> CREATE_SUSPENDED, NULL, NULL, &si, processInfo);
>   
>   Kernel32Handle = LoadLibrary("KERNEL32.DLL");
>   if (Kernel32Handle != NULL)
> --- 1637,1650 
>   AddUserToTokenDacl(restrictedToken);
>   #endif
>   
> ! flags = CREATE_SUSPENDED;
> ! 
> ! /* Protect console process from Ctrl-C */
> ! if (!as_service) {
> ! flags |= CREATE_NEW_PROCESS_GROUP;
> ! }
> ! 
> ! r = CreateProcessAsUser(restrictedToken, NULL, cmd, NULL, NULL, TRUE, 
> flags, NULL, NULL, &si, processInfo);
>   
>   Kernel32Handle = LoadLibrary("KERNEL32.DLL");
>   if (Kernel32Handle != NULL)

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


-- 
  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] Negative Transition Aggregate Functions (WIP)

[Florian Pflug]

On Apr10, 2014, at 02:13 , Florian Pflug  wrote:
> On Apr9, 2014, at 23:17 , Florian Pflug  wrote:
>> On Apr9, 2014, at 21:35 , Tom Lane  wrote:
>>> A quick test says that avg(int4)
>>> is about five percent slower than sum(int4), so that's the kind of hit
>>> we'd be taking on non-windowed aggregations if we do it like this.
>> 
>> That's rather surprising though. AFAICS, there's isn't much difference
>> between the two transfer functions int4_sum and int4_avg_accum at all.
>> 
>> The differences come down to (+ denoting things which ought to make
>> int4_avg_accum slower compared to int4_sum, - denoting the opposite)
>> 
>> 1. +) int4_avg_accum calls AggCheckCallContext
>> 2. -) int4_sum checks if the state is NULL (it never is for int4_avg_accum)
>> 3. +) int4_avg_accum uses ARR_HASNULL, ARR_SIZE, ARR_OVERHEAD_NONULLS
>>  to verify that the state is a 2-element array without NULL entries
>> 4. -) int4_sum checks if the input is NULL
> 
> I've done a bit of profiling on this (using Instruments.app on OSX). One
> thing I missed is that inv4_avg_accum also calls pg_detoast_datum - that
> calls comes from the PG_GETARG_ARRAYTYPE_P macro. Doing that is a bit silly,
> since we know that the datum cannot possibly be toasted I think (or if it
> was, nodeAgg.c should do the de-toasting).
> 
> The profile also attributes a rather large percent of the total runtime of
> int4_avg_accum (around 30%) to the call of AggCheckCallContext(). Getting
> rid of that would help quite a few transition functions, invertible or not.
> That certainly seems doable - we'd need a way to mark functions as internal
> support functions, and prevent user-initiated calls of such functions. 
> Transition functions marked that way could then safely scribble over their
> state arguments without having to consult AggCheckCallContext() first.
> 
> ...
> 
> However, I still believe the best approach at this point is to just work
> on making int4_avg_accum faster. I still see no principal reason what it
> has to be noticeably slower - the only additional work it absolutely *has*
> to perform is *one* 64-bit increment.

I played with this a bit.

Currently, int4_avg_accum invokes AggCheckCallContext every time, and also
repeatedly checks whether the array has the required dimension - which,
incidentally, is the only big difference between int4_avg_accum and int4_sum.

To avoid that, I added a flags field to fcinfo which nodeAgg uses to tell
transition functions whether they're called for the first time, or are being
called with whatever state they themselves returned last time, i.e the
n-th time.

If the n-th time flag is set, int4_avg_accum simply retrieves the state with
PG_GETARG_DATUM() instead of PG_GETARG_ARRAYTYPE_P(), relying on the fact
that it never returns toasted datums itself, and also doesn't bother to validate
the array size, for the same reason.

If the flag is not set, it uses PG_GETARG_ARRAYTYPE_COPY_P and does validate
the array size. In theory, it could further distinguish between a 1st call
in an aggregation context (where the copy is unnecessary), and a user-initiated
call (i.e. outside an aggregation). But that seems unnecessary - one additional
copy per aggregation group seems unlikely to be a problem.

With this in place, instruction-level profiling using Apple's Instruments.app
shows that int4_avg_accum takes about 1.5% of the total runtime of a simple
aggregation, while int4_sum takes about 1.2%. 

A (very rough) patch is attached.

I haven't been able to repeat Tom's measurement which shows a 5% difference in
performance between the invertible and the non-invertible versions of SUM(int4),
so I cannot say if this removes that. But the profiling I've done would 
certainly
indicate it should.

best regards,
Florian Pflug



invtrans_sumint4_opt2.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] trailing comment ghost-timing

[Bruce Momjian]

On Mon, Mar 31, 2014 at 02:06:28PM -0400, Bruce Momjian wrote:
> Where are we on this?  It seem odd that psql sends /* */ comments to the
> server, but not "--" comments.  Should this be documented or changed?
> 
> I am confused why changing the behavior would affect the regression test
> output as -- and /* */ comments already appear, and it was stated that
> "--" comments are already not sent to the server.
> 
> Everyone agreed that suppressing \timing output for a PGRES_EMPTY_QUERY
> return result was not a good idea.

I have applied the attached document patch to document that '--'
comments are not passed to the server, while C-style block comments are.
We can call this a feature.  ;-)

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

  + Everyone has their own god. +
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
new file mode 100644
index 5dce06a..85899d7
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
*** testdb=>
*** 679,684 
--- 679,690 
   and
  .
  
+ 
+ 
+ While C-style block comments are passed to the server for
+ processing and removal, SQL-standard comments are removed by
+ psql.
+ 

  


-- 
Sent 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 match fix for fast scan

[Heikki Linnakangas]

On 04/10/2014 10:00 PM, Alexander Korotkov wrote:

On Thu, Apr 10, 2014 at 8:22 PM, Fabrízio de Royes Mello <
fabriziome...@gmail.com> wrote:


On Thu, Apr 10, 2014 at 11:09 AM, Alexander Korotkov 
wrote:



GIN partial match appears to be broken after fast scan. Following simple
test case raises assertion failure.

create extension btree_gin;
create table test as (select id, random() as val from
generate_series(1,100) id);
create index test_idx on test using gin (val);
vacuum test;
select * from test where val between 0.1 and 0.9;

Attached patch fixes bugs in entryGetItem function.
I would especially point that "continue;" checks "while" condition even
if it's postfix "while". That's why I surrounded tbm_iterate with another
"while".



Interesting... to me (using current master) your test case doesn't fail...

fabrizio=# select * from test where val between 0.1 and 0.9;
id   |val
+---
   1 | 0.554413774050772
   2 | 0.767866868525743
   3 | 0.601187175605446
...


But fail if I change the values of between clause:

fabrizio=# select * from test where val between 0.1 and 0.19;
ERROR:  tuple offset out of range: 8080




It must be compiled with --enable-cassert to fail on assertion.


I'm actually getting the "tuple offset out of range" with Fabrizio's 
query, even after your fix (not every time, run it a few times, 
launching a new connection each time). So there's another bug lurking 
there. The problem seems to be that even though we've checked in the 
innermost loop that not all of the items on the page are <= advancePast, 
that situation can change later if advancePast is advanced. So on next 
invocation entryGetItem, the loop to skip past advancePast might read 
bogus offsets in the array.


I pushed a patch, which includes Alexander's fix, and also fixes the 
second issue.


- 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] Problem with displaying "wide" tables in psql

[Sergey Muraviov]

Hi.

Thanks for your tests.

I've fixed problem with headers, but got new one with data.
I'll try to solve it tomorrow.


2014-04-10 18:45 GMT+04:00 Greg Stark :

> Ok, So I've hacked on this a bit. Below is a test case showing the
> problems I've found.
>
> 1) It isn't using the "newline" and "wrap" indicators or dividing lines.
>
> 2) The header is not being displayed properly when it contains a newline.
>
> I can hack in the newline and wrap indicators but the header
> formatting requires reworking the logic a bit. The header and data
> need to be stepped through in parallel rather than having a loop to
> handle the wrapping within the handling of a single line. I don't
> really have time for that today but if you can get to it that would be
> fine,
>



-- 
Best regards,
Sergey Muraviov

Re: [HACKERS] [PATCH] Negative Transition Aggregate Functions (WIP)

[Dean Rasheed]

On 10 April 2014 19:54, Tom Lane  wrote:
> Dean Rasheed  writes:
>> On 10 April 2014 19:04, Tom Lane  wrote:
>>> What about names for the invertible-aggregate infrastructure?
>>> I'm tempted to prefix "inv" to all the existing names, but then
>>> "invsfunc" means the alternate forward function ... can we use
>>> "invifunc" for the inverse transition function?  Or maybe the
>>> prefix should be just "i".
>
>> Hmm, I'm not a fan of any of those names. Perhaps "win" as a prefix to
>> denote a sliding window? Or just "m" for "moving aggregate".
>
> Hmm ... "moving aggregate" is actually a pretty good name for this
> whole feature -- better than "invertible aggregate" anyway.  I can
> feel a global-search-and-replace coming on.
>
> So if we go with that terminology, perhaps these names for the
> new CREATE AGGREGATE parameters:
>
> initfuncapplies to plain aggregation, mutually exclusive with initcond
> msfunc  (or just mfunc?) forward transition for moving-agg mode
> mifunc  inverse transition for moving-agg mode
> mstype  state datatype for moving-agg mode
> msspace space estimate for mstype
> mfinalfunc  final function for moving-agg mode
> minitfunc   "firsttrans" for moving-agg mode
> minitcond   mutually exclusive with minitfunc
>

Yeah, those work for me.

I think I prefer "mfunc" to "msfunc", but perhaps that's just my
natural aversion to the "ms" prefix :-)

Also, perhaps "minvfunc" rather than "mifunc" because "i" by itself
could mean "initial".

Regards,
Dean


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

Re: [HACKERS] Partial match fix for fast scan

[Alexander Korotkov]

On Thu, Apr 10, 2014 at 8:22 PM, Fabrízio de Royes Mello <
fabriziome...@gmail.com> wrote:

>
> On Thu, Apr 10, 2014 at 11:09 AM, Alexander Korotkov  > wrote:
>
>> Hi,
>>
>> GIN partial match appears to be broken after fast scan. Following simple
>> test case raises assertion failure.
>>
>> create extension btree_gin;
>> create table test as (select id, random() as val from
>> generate_series(1,100) id);
>> create index test_idx on test using gin (val);
>> vacuum test;
>> select * from test where val between 0.1 and 0.9;
>>
>> Attached patch fixes bugs in entryGetItem function.
>> I would especially point that "continue;" checks "while" condition even
>> if it's postfix "while". That's why I surrounded tbm_iterate with another
>> "while".
>>
>>
> Interesting... to me (using current master) your test case doesn't fail...
>
> fabrizio=# select * from test where val between 0.1 and 0.9;
>id   |val
> +---
>   1 | 0.554413774050772
>   2 | 0.767866868525743
>   3 | 0.601187175605446
> ...
>
>
> But fail if I change the values of between clause:
>
> fabrizio=# select * from test where val between 0.1 and 0.19;
> ERROR:  tuple offset out of range: 8080
>


It must be compiled with --enable-cassert to fail on assertion.

--
With best regards,
Alexander Korotkov.

Re: [HACKERS] [PATCH] Negative Transition Aggregate Functions (WIP)

[Tom Lane]

Dean Rasheed  writes:
> On 10 April 2014 19:04, Tom Lane  wrote:
>> What about names for the invertible-aggregate infrastructure?
>> I'm tempted to prefix "inv" to all the existing names, but then
>> "invsfunc" means the alternate forward function ... can we use
>> "invifunc" for the inverse transition function?  Or maybe the
>> prefix should be just "i".

> Hmm, I'm not a fan of any of those names. Perhaps "win" as a prefix to
> denote a sliding window? Or just "m" for "moving aggregate".

Hmm ... "moving aggregate" is actually a pretty good name for this
whole feature -- better than "invertible aggregate" anyway.  I can
feel a global-search-and-replace coming on.

So if we go with that terminology, perhaps these names for the
new CREATE AGGREGATE parameters:

initfuncapplies to plain aggregation, mutually exclusive with initcond
msfunc  (or just mfunc?) forward transition for moving-agg mode
mifunc  inverse transition for moving-agg mode
mstype  state datatype for moving-agg mode
msspace space estimate for mstype
mfinalfunc  final function for moving-agg mode
minitfunc   "firsttrans" for moving-agg mode
minitcond   mutually exclusive with minitfunc

That takes us up to 16 columns in pg_aggregate, but it's still not going
to be a very voluminous catalog --- there's only 171 rows there today.
So I'm not particularly concerned about space, and if there's a chance
of squeezing out cycles, I think we should seize 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] [PATCH] Negative Transition Aggregate Functions (WIP)

[Dean Rasheed]

On 10 April 2014 19:04, Tom Lane  wrote:
> Dean Rasheed  writes:
>> On 10 April 2014 15:18, Tom Lane  wrote:
>>> This idea of a separate firsttrans function is interesting but perhaps
>>> orthogonal to the current patch.  Also, I don't quite understand how
>>> it would work for aggregates with null initvalues; don't you end up
>>> with exactly the same conflict about how you can't mark the transfn
>>> strict?  Or is the idea that firsttrans would *only* apply to aggregates
>>> with null initvalue, and so you wouldn't even pass the previous state
>>> value to it?
>
>> I was imagining that firsttrans would only be passed the first value
>> to be aggregated, not any previous state, and that it would be illegal
>> to specify both an initcond and a firsttrans function.
>
> Got it.  So the existing behavior where we insert the first non-null
> value could be seen as a degenerate case in which the firsttrans function
> is an identity.
>

Right.

>> The forward transition function would only be called for values after
>> the first, by which point the state would be non-null, and so it could
>> be made strict in most cases. The same would apply to the invertible
>> transition functions, so they wouldn't have to do null counting, which
>> in turn would make their state types simpler.
>
> Makes sense to me.  We need names for these things though.  I don't
> think abbreviating to "ffunc" is a good idea because of the likelihood
> of confusion with the finalfunc (indeed I see the CREATE AGGREGATE
> ref page is already using "ffunc" as a short form for finalfunc).
> Maybe "initfunc", which would parallel "initcond"?
>

Yes, I was already thinking that "initfunc" is actually a much better
name for that.

> What about names for the invertible-aggregate infrastructure?
> I'm tempted to prefix "inv" to all the existing names, but then
> "invsfunc" means the alternate forward function ... can we use
> "invifunc" for the inverse transition function?  Or maybe the
> prefix should be just "i".
>

Hmm, I'm not a fan of any of those names. Perhaps "win" as a prefix to
denote a sliding window? Or just "m" for "moving aggregate".

Regards,
Dean


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

Re: [HACKERS] [PATCH] Negative Transition Aggregate Functions (WIP)

[Tom Lane]

Dean Rasheed  writes:
> On 10 April 2014 15:18, Tom Lane  wrote:
>> This idea of a separate firsttrans function is interesting but perhaps
>> orthogonal to the current patch.  Also, I don't quite understand how
>> it would work for aggregates with null initvalues; don't you end up
>> with exactly the same conflict about how you can't mark the transfn
>> strict?  Or is the idea that firsttrans would *only* apply to aggregates
>> with null initvalue, and so you wouldn't even pass the previous state
>> value to it?

> I was imagining that firsttrans would only be passed the first value
> to be aggregated, not any previous state, and that it would be illegal
> to specify both an initcond and a firsttrans function.

Got it.  So the existing behavior where we insert the first non-null
value could be seen as a degenerate case in which the firsttrans function
is an identity.

> The forward transition function would only be called for values after
> the first, by which point the state would be non-null, and so it could
> be made strict in most cases. The same would apply to the invertible
> transition functions, so they wouldn't have to do null counting, which
> in turn would make their state types simpler.

Makes sense to me.  We need names for these things though.  I don't
think abbreviating to "ffunc" is a good idea because of the likelihood
of confusion with the finalfunc (indeed I see the CREATE AGGREGATE
ref page is already using "ffunc" as a short form for finalfunc).
Maybe "initfunc", which would parallel "initcond"?

What about names for the invertible-aggregate infrastructure?
I'm tempted to prefix "inv" to all the existing names, but then
"invsfunc" means the alternate forward function ... can we use
"invifunc" for the inverse transition function?  Or maybe the
prefix should be just "i".

regards, tom lane


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

Re: [HACKERS] [PATCH] Negative Transition Aggregate Functions (WIP)

[Dean Rasheed]

On 10 April 2014 15:18, Tom Lane  wrote:
> Dean Rasheed  writes:
>> On 10 April 2014 01:13, Florian Pflug  wrote:
>>> However, I still believe the best approach at this point is to just work
>>> on making int4_avg_accum faster. I still see no principal reason what it
>>> has to be noticeably slower - the only additional work it absolutely *has*
>>> to perform is *one* 64-bit increment.
>
>> In the best case that would make sum() not noticeably slower than
>> avg(), whereas using a firsttrans/initialfunction would potentially
>> make both of them faster than they currently are, and not just in
>> window queries.
>
> I'm still of the opinion that we should separate the transfn for
> invertible cases from the normal one, and allow for two separate
> state types.  One of the things that helps with is the strictness
> consideration: you no longer have to have the same strictness
> setting for the plain and invertible forward transfns.
>

Yes, I can imagine that there would be some aggregates for which the
plain forwards transition function would be simpler than the
invertible one, with a simpler state type. You'd still be left with
quite a large number of existing aggregates having non-strict plain
transition functions, in addition to a bunch of new non-strict
invertible transition functions that had to do null counting.


> This idea of a separate firsttrans function is interesting but perhaps
> orthogonal to the current patch.  Also, I don't quite understand how
> it would work for aggregates with null initvalues; don't you end up
> with exactly the same conflict about how you can't mark the transfn
> strict?  Or is the idea that firsttrans would *only* apply to aggregates
> with null initvalue, and so you wouldn't even pass the previous state
> value to it?
>

I was imagining that firsttrans would only be passed the first value
to be aggregated, not any previous state, and that it would be illegal
to specify both an initcond and a firsttrans function.

The forward transition function would only be called for values after
the first, by which point the state would be non-null, and so it could
be made strict in most cases. The same would apply to the invertible
transition functions, so they wouldn't have to do null counting, which
in turn would make their state types simpler.

Regards,
Dean


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

Re: [HACKERS] psql \d+ and oid display

[Bruce Momjian]

On Thu, Apr 10, 2014 at 01:05:32PM -0400, Greg Stark wrote:
> If it's conditional I think "when it matches a guc" is too hard for users to
> use.

Yes, we gave up on having the OID display match the GUC;  we just
display something if and only if it oids are present.

Robert is talking about the "Identity Replica" display, which is new in
9.4 and should match how we display oids.  Right now "Identity Replica"
only displays something if the user changed the default.

> I think "say nothing if oids are off and say something of their on" would be
> fine. It would result in clutter for users which oids on by default but that's
> a non default setting.

Yes, that is what the proposed patch does.

-- 
  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] psql \d+ and oid display

[Greg Stark]

If it's conditional I think "when it matches a guc" is too hard for users
to use.

I think "say nothing if oids are off and say something of their on" would
be fine. It would result in clutter for users which oids on by default but
that's a non default setting.

And the consequences of having oids on when they're intended to be off are
much more likely to be missed our forgotten and need a reminder. If they're
off when they need to be on you'll surely notice...

-- 
greg
On 10 Apr 2014 12:45, "Bruce Momjian"  wrote:

> On Thu, Apr 10, 2014 at 12:23:40PM -0400, Robert Haas wrote:
> > > What might make more sense is this:
> > >
> > > if ((tableinfo.relkind == 'r' || tableinfo.relkind == 'm') &&
> > > /*
> > >  * No need to display default values;  we already display a
> > >  * REPLICA IDENTITY marker on indexes.
> > >  */
> > > tableinfo.relreplident != 'i' &&
> > > ((strcmp(schemaname, "pg_catalog") != 0 &&
> tableinfo.relreplident != 'd') ||
> > >  (strcmp(schemaname, "pg_catalog") == 0 &&
> tableinfo.relreplident != 'n')))
> >
> > Well, this is why I think we should just display it always.  I don't
> > think users are going to remember the exact algorithm for whether or
> > not the line gets displayed, so you're just putting yourself in a
> > situation where the \d+ output doesn't actually inform the user.  If
> > you want to leave it out when it's "default" and show the "none" line
> > for catalog tables, that's OK by me too.  But calling one line of
> > output that displays important information "clutter" and only appears
> > when the user explicitly requests verbose mode (with \d+ rather than
> > \d) strikes me as as silly.
>
> The issue is that "none" is the default for system tables and "default"
> is the default for user tables.  Tom complained about the "none" for the
> pg_depend display.
>
> I am starting to think I am not going to make everyone happy and we just
> need to vote.  Frankly, I think there are enough people who want this
> conditionally displayed that I don't even need a vote.
>
> --
>   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] Minor performance improvement in transition to external sort

[Simon Riggs]

On 6 February 2014 18:21, Jeff Janes  wrote:
> On Tue, Feb 4, 2014 at 2:22 PM, Jeremy Harris  wrote:
>>
>> The attached patch replaces the existing siftup method for heapify with
>> a siftdown method. Tested with random integers it does 18% fewer
>> compares and takes 10% less time for the heapify, over the work_mem
>> range 1024 to 1048576.
>
>
> Thanks for working on this.

+1

Your patch isn't linked properly from the CF manager though.

If you like patches like this then there's a long(er) list of
optimizations already proposed previously around sorting. It would be
good to have someone work through them for external sorts. I believe
Noah is working on parallel internal sort (as an aside).

There's also an optimization possible for merge joins where we use the
output of the first sort as an additional filter on the second sort.
That can help when we're going to join two disjoint tables.

-- 
 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] psql \d+ and oid display

[Bruce Momjian]

On Thu, Apr 10, 2014 at 12:23:40PM -0400, Robert Haas wrote:
> > What might make more sense is this:
> >
> > if ((tableinfo.relkind == 'r' || tableinfo.relkind == 'm') &&
> > /*
> >  * No need to display default values;  we already display a
> >  * REPLICA IDENTITY marker on indexes.
> >  */
> > tableinfo.relreplident != 'i' &&
> > ((strcmp(schemaname, "pg_catalog") != 0 && 
> > tableinfo.relreplident != 'd') ||
> >  (strcmp(schemaname, "pg_catalog") == 0 && 
> > tableinfo.relreplident != 'n')))
> 
> Well, this is why I think we should just display it always.  I don't
> think users are going to remember the exact algorithm for whether or
> not the line gets displayed, so you're just putting yourself in a
> situation where the \d+ output doesn't actually inform the user.  If
> you want to leave it out when it's "default" and show the "none" line
> for catalog tables, that's OK by me too.  But calling one line of
> output that displays important information "clutter" and only appears
> when the user explicitly requests verbose mode (with \d+ rather than
> \d) strikes me as as silly.

The issue is that "none" is the default for system tables and "default"
is the default for user tables.  Tom complained about the "none" for the
pg_depend display.

I am starting to think I am not going to make everyone happy and we just
need to vote.  Frankly, I think there are enough people who want this
conditionally displayed that I don't even need a vote.

-- 
  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] psql \d+ and oid display

[Robert Haas]

On Wed, Apr 9, 2014 at 11:42 AM, Bruce Momjian  wrote:
> On Wed, Apr  9, 2014 at 09:27:11AM -0400, Robert Haas wrote:
>> On Wed, Apr 9, 2014 at 1:02 AM, Tom Lane  wrote:
>> > Robert Haas  writes:
>> >> Well, that's sorta my concern.  I mean, right now we've got people
>> >> saying "what the heck is a replica identity?".  But, if the logical
>> >> decoding stuff becomes popular, as I hope it will, that's going to be
>> >> an important thing for people to adjust, and the information needs to
>> >> be present in a clear and easily-understood way.  I haven't studied
>> >> the current code in detail so maybe it's fine.  I just want to make
>> >> sure we're not giving it second-class treatment solely on the basis
>> >> that it's new and people aren't using it yet.
>> >
>> > I think the proposal is "don't mention the property if it has the
>> > default value".  That's not second-class status, as long as people
>> > who know what the property is understand that behavior.  It's just
>> > conserving screen space.
>>
>> One thing that concerns me is that replica identity has a different
>> default for system tables (NOTHING) than for other tables (DEFAULT).
>> So when we say we're not going to display the default value, are we
>> going to display it when it's not NOTHING, when it's not DEFAULT, or
>> when it's not the actual default for that particular kind of table?
>
> We exclude pg_catalog from displaying Replica Identity due to this
> inconsistency.  I assume this was desired because you can't replicate
> system tables.  Is that true?  The current test is:
>
> if ((tableinfo.relkind == 'r' || tableinfo.relkind == 'm') &&
> /*
>  * No need to display default values;  we already display a
>  * REPLICA IDENTITY marker on indexes.
>  */
> tableinfo.relreplident != 'd' && tableinfo.relreplident != 'i' &&
> strcmp(schemaname, "pg_catalog") != 0)
>
> What might make more sense is this:
>
> if ((tableinfo.relkind == 'r' || tableinfo.relkind == 'm') &&
> /*
>  * No need to display default values;  we already display a
>  * REPLICA IDENTITY marker on indexes.
>  */
> tableinfo.relreplident != 'i' &&
> ((strcmp(schemaname, "pg_catalog") != 0 && tableinfo.relreplident 
> != 'd') ||
>  (strcmp(schemaname, "pg_catalog") == 0 && tableinfo.relreplident 
> != 'n')))

Well, this is why I think we should just display it always.  I don't
think users are going to remember the exact algorithm for whether or
not the line gets displayed, so you're just putting yourself in a
situation where the \d+ output doesn't actually inform the user.  If
you want to leave it out when it's "default" and show the "none" line
for catalog tables, that's OK by me too.  But calling one line of
output that displays important information "clutter" and only appears
when the user explicitly requests verbose mode (with \d+ rather than
\d) strikes me as as silly.

-- 
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] Partial match fix for fast scan

[Fabrízio de Royes Mello]

On Thu, Apr 10, 2014 at 11:09 AM, Alexander Korotkov
wrote:

> Hi,
>
> GIN partial match appears to be broken after fast scan. Following simple
> test case raises assertion failure.
>
> create extension btree_gin;
> create table test as (select id, random() as val from
> generate_series(1,100) id);
> create index test_idx on test using gin (val);
> vacuum test;
> select * from test where val between 0.1 and 0.9;
>
> Attached patch fixes bugs in entryGetItem function.
> I would especially point that "continue;" checks "while" condition even if
> it's postfix "while". That's why I surrounded tbm_iterate with another
> "while".
>
>
Interesting... to me (using current master) your test case doesn't fail...

fabrizio=# select * from test where val between 0.1 and 0.9;
   id   |val
+---
  1 | 0.554413774050772
  2 | 0.767866868525743
  3 | 0.601187175605446
...


But fail if I change the values of between clause:

fabrizio=# select * from test where val between 0.1 and 0.19;
ERROR:  tuple offset out of range: 8080


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] Problem with displaying "wide" tables in psql

[Greg Stark]

Ok, So I've hacked on this a bit. Below is a test case showing the
problems I've found.

1) It isn't using the "newline" and "wrap" indicators or dividing lines.

2) The header is not being displayed properly when it contains a newline.

I can hack in the newline and wrap indicators but the header
formatting requires reworking the logic a bit. The header and data
need to be stepped through in parallel rather than having a loop to
handle the wrapping within the handling of a single line. I don't
really have time for that today but if you can get to it that would be
fine,
postgres=***# \pset
Border style (border) is 2.
Target width (columns) is 20.
Expanded display (expanded) is on.
Field separator (fieldsep) is "|".
Default footer (footer) is on.
Output format (format) is wrapped.
Line style (linestyle) is unicode.
Null display (null) is "".
Locale-adjusted numeric output (numericlocale) is off.
Pager (pager) usage is off.
Record separator (recordsep) is .
Table attributes (tableattr) unset.
Title (title) unset.
Tuples only (tuples_only) is off.

postgres=***# select array_to_string(array_agg(repeat('x',n)),E'\n') as "x
y",  array_to_string(array_agg(repeat('yy',10-n)),E'\n') as "x
y
z" from (select generate_series(1,10)) as t(n);

┌─[ RECORD 1 ]─┐
│ x │ x│
│ y │ xx   │
│   │ xxx  │
│   │  │
│   │ x│
│   │ xx   │
│   │ xxx  │
│   │  │
│   │ x│
│   │ xx   │
│ x │  │
│   │ yy   │
│ y │  │
│   │  │
│ z │  │
│   │ yy   │
│   │  │
│   │ yy   │
│   │  │
│   │ yy   │
│   │  │
│   │ yy   │
│   │  │
└───┴──┘

postgres=***# \pset linestyle ascii
Line style (linestyle) is ascii.

postgres=***# select array_to_string(array_agg(repeat('x',n)),E'\n') as "x
y",  array_to_string(array_agg(repeat('yy',10-n)),E'\n') as "x
y
z" from (select generate_series(1,10)) as t(n);

+-[ RECORD 1 ]-+
| x | x|
| y | xx   |
|   | xxx  |
|   |  |
|   | x|
|   | xx   |
|   | xxx  |
|   |  |
|   | x|
|   | xx   |
| x |  |
|   | yy   |
| y |  |
|   |  |
| z |  |
|   | yy   |
|   |  |
|   | yy   |
|   |  |
|   | yy   |
|   |  |
|   | yy   |
|   |  |
+---+--+

postgres=***# \pset columns 30
Target width (columns) is 30.

postgres=***# \pset linestyle unicode
Line style (linestyle) is unicode.

postgres=***# \pset columns 30
Target width (columns) is 30.

postgres=***# \pset expanded off
Expanded display (expanded) is off.

postgres=***# select array_to_string(array_agg(repeat('x',n)),E'\n') as "x
y",  array_to_string(array_agg(repeat('yy',10-n)),E'\n') as "x
y
z" from (select generate_series(1,10)) as t(n);

┌───┬┐
│ x↵│   x   ↵│
│ y │   y   ↵│
│   │   z│
├───┼┤
│ x↵│ yy…│
│ xx   ↵│…  ↵│
│ xxx  ↵│ yy…│
│  ↵│…yy↵│
│ x↵│ yy↵│
│ xx   ↵│   ↵│
│ xxx  ↵│ yy↵│
│  ↵│   ↵│
│ x↵│ yy↵│
│ x…│   ↵│
│…x │ yy↵│
│   ││
└───┴┘
(1 row)



postgres=***# \pset linestyle ascii
Line style (linestyle) is ascii.

postgres=***# select array_to_string(array_agg(repeat('x',n)),E'\n') as "x
y",  array_to_string(array_agg(repeat('yy',10-n)),E'\n') as "x
y
z" from (select generate_series(1,10)) as t(n);

+---++
| x+|   x   +|
| y |   y   +|
|   |   z|
+---++
| x+| yy.|
| xx   +|.  +|
| xxx  +| yy.|
|  +|.yy+|
| x+| yy+|
| xx   +|   +|
| xxx  +| yy+|
|  +|   +|
| x+| yy+|
| x.|   +|
|.x | yy+|
|   ||
+---++

-- 
Sent 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] Negative Transition Aggregate Functions (WIP)

[Tom Lane]

Dean Rasheed  writes:
> On 10 April 2014 01:13, Florian Pflug  wrote:
>> However, I still believe the best approach at this point is to just work
>> on making int4_avg_accum faster. I still see no principal reason what it
>> has to be noticeably slower - the only additional work it absolutely *has*
>> to perform is *one* 64-bit increment.

> In the best case that would make sum() not noticeably slower than
> avg(), whereas using a firsttrans/initialfunction would potentially
> make both of them faster than they currently are, and not just in
> window queries.

I'm still of the opinion that we should separate the transfn for
invertible cases from the normal one, and allow for two separate
state types.  One of the things that helps with is the strictness
consideration: you no longer have to have the same strictness
setting for the plain and invertible forward transfns.

This idea of a separate firsttrans function is interesting but perhaps
orthogonal to the current patch.  Also, I don't quite understand how
it would work for aggregates with null initvalues; don't you end up
with exactly the same conflict about how you can't mark the transfn
strict?  Or is the idea that firsttrans would *only* apply to aggregates
with null initvalue, and so you wouldn't even pass the previous state
value to 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] Adding unsigned 256 bit integers

[k...@rice.edu]

On Thu, Apr 10, 2014 at 09:13:47PM +0800, Olivier Lalonde wrote:
> I was wondering if there would be any way to do the following in PostgreSQL:
> 
> UPDATE cryptotable SET work = work + 'some big hexadecimal number'
> 
> where work is an unsigned 256 bit integer. Right now my column is a
> character varying(64) column (hexadecimal representation of the number) but
> I would be happy to switch to another data type if it lets me do the
> operation above.
> 
> If it's not possible with vanilla PostgreSQL, are there extensions that
> could help me?
> 
> -- 
> - Oli
> 
> Olivier Lalonde
> http://www.syskall.com <-- connect with me!
> 

Hi Olivier,

Here are some sample pl/pgsql helper functions that I have written for
other purposes. They use integers but can be adapted to use numeric.

Regards,
Ken
---
CREATE OR REPLACE FUNCTION hex2dec(t text) RETURNS integer AS $$
DECLARE
  r RECORD;
BEGIN
  FOR r IN EXECUTE 'SELECT x'''||t||'''::integer AS hex' LOOP
RETURN r.hex;
  END LOOP;
END
$$ LANGUAGE plpgsql IMMUTABLE STRICT;
---

---
CREATE OR REPLACE FUNCTION bytea2int (
  in_string BYTEA
) RETURNS INTEGER AS $$

DECLARE

  b1 INTEGER := 0;
  b2 INTEGER := 0;
  b3 INTEGER := 0;
  b4 INTEGER := 0;
  out_int INTEGER := 0;

BEGIN

  CASE OCTET_LENGTH(in_string)
WHEN 1 THEN
  b4 := get_byte(in_string, 0);
WHEN 2 THEN
  b3 := get_byte(in_string, 0);
  b4 := get_byte(in_string, 1);
WHEN 3 THEN
  b2 := get_byte(in_string, 0);
  b3 := get_byte(in_string, 1);
  b4 := get_byte(in_string, 2);
WHEN 4 THEN
  b1 := get_byte(in_string, 0);
  b2 := get_byte(in_string, 1);
  b3 := get_byte(in_string, 2);
  b4 := get_byte(in_string, 3);
  END CASE;

  out_int := (b1 << 24) + (b2 << 16) + (b3 << 8) + b4;

  RETURN(out_int);
END;
$$ LANGUAGE plpgsql IMMUTABLE;
---


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

[HACKERS] Partial match fix for fast scan

[Alexander Korotkov]

Hi,

GIN partial match appears to be broken after fast scan. Following simple
test case raises assertion failure.

create extension btree_gin;
create table test as (select id, random() as val from
generate_series(1,100) id);
create index test_idx on test using gin (val);
vacuum test;
select * from test where val between 0.1 and 0.9;

Attached patch fixes bugs in entryGetItem function.
I would especially point that "continue;" checks "while" condition even if
it's postfix "while". That's why I surrounded tbm_iterate with another
"while".

--
With best regards,
Alexander Korotkov.


ginfastscanfix.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] Adding unsigned 256 bit integers

[Craig Ringer]

On 04/10/2014 09:13 PM, Olivier Lalonde wrote:
> I was wondering if there would be any way to do the following in PostgreSQL:
> 
> UPDATE cryptotable SET work = work + 'some big hexadecimal number'

For readers finding this in the archives, this question also appears here:

http://dba.stackexchange.com/questions/62934/adding-unsigned-256-bit-integers-in-postgresql

-- 
 Craig Ringer   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] Adding unsigned 256 bit integers

[Andrew Dunstan]

On 04/10/2014 09:13 AM, Olivier Lalonde wrote:
I was wondering if there would be any way to do the following in 
PostgreSQL:


UPDATE cryptotable SET work = work + 'some big hexadecimal number'

where work is an unsigned 256 bit integer. Right now my column is a 
character varying(64) column (hexadecimal representation of the 
number) but I would be happy to switch to another data type if it lets 
me do the operation above.


If it's not possible with vanilla PostgreSQL, are there extensions 
that could help me?






The numeric type allows numbers with huge numbers of digits. I've used 
it to calculate fibonacci numbers thousands of digits long.


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

[Tom Lane]

Etsuro Fujita  writes:
> (2014/04/10 0:08), Tom Lane wrote:
>> TBH I think that's barely the tip of the iceberg of cases where this
>> patch will get the wrong answer.

>> Also, I don't see it doing anything to check the ordering
>> of multiple index columns

> I think that the following code in index_pathkeys_are_extensible() would
> check the ordering:
> + if (!pathkeys_contained_in(pathkeys, root->query_pathkeys))
> + return false;

Hm ... if you're relying on that, then what's the point of the new loop
at all?

regards, tom lane


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

[HACKERS] Adding unsigned 256 bit integers

[Olivier Lalonde]

I was wondering if there would be any way to do the following in PostgreSQL:

UPDATE cryptotable SET work = work + 'some big hexadecimal number'

where work is an unsigned 256 bit integer. Right now my column is a
character varying(64) column (hexadecimal representation of the number) but
I would be happy to switch to another data type if it lets me do the
operation above.

If it's not possible with vanilla PostgreSQL, are there extensions that
could help me?

-- 
- Oli

Olivier Lalonde
http://www.syskall.com <-- connect with me!

Freelance web and Node.js engineer
Skype: o-lalonde

Re: [HACKERS] [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of < 0

[Bruce Momjian]

On Thu, Apr 10, 2014 at 11:05:49AM +0530, Amit Kapila wrote:
> On Tue, Apr 8, 2014 at 11:32 PM, Bruce Momjian  wrote:
> > On Sun, Apr  6, 2014 at 11:45:59AM +0530, Amit Kapila wrote:
> > In fact, this C program compiled by gcc on Debian issues no compiler
> > warnings and returns 'hello', showing that -1 and ~0 compare as equal:
> >
> > int
> > main(int argc, char **argv)
> > {
> > int i;
> > unsigned int j;
> >
> > i = -1;
> > j = ~0;
> >
> > if (i == j)
> > printf("hello\n");
> >
> > return 0;
> > }
> 
> I have add below code to check it's usage as per PG:
> 
> if (j < 0)
> printf("hello-1\n");
> 
> It doesn't print hello-1, which means that all the check's in code
> for  < 0 can have problem.

Ah, yes, good point.  This is going to require backpatching then.

> >> 1.
> >> int
> >> pg_foreach_ifaddr(PgIfAddrCallback callback, void *cb_data)
> >> sock = WSASocket(AF_INET, SOCK_DGRAM, 0, 0, 0, 0);
> >> if (sock == SOCKET_ERROR)
> >
> > Well, the actual problem here is that WSASocket() returns INVALID_SOCKET
> > per the documentation, not SOCKET_ERROR.  I did not use PGINVALID_SOCKET
> > here because this is Windows-specific code, defining 'sock' as SOCKET.
> > We could have sock defined as pgsocket, but because this is Windows code
> > already, it doesn't seem wise to mix portability code in there.
> 
> I think it's better to use check like below, just for matter of
> consistency with other place
> if (sock == INVALID_SOCKET)

Agreed.  That is how I have coded the patch.

-- 
  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] WIP patch (v2) for updatable security barrier views

[Gregory Smith]

On 4/9/14 9:56 PM, Stephen Frost wrote:

As for docs and testing, those are things we would certainly be better
off with and may mean that this isn't able to make it into 9.4, which is
fair, but I wouldn't toss it out solely due to that.


We have a git repo with multiple worked out code examples, ones that 
have been in active testing for months now.  It's private just to reduce 
mistakes as things are cleared for public consumption, but I (and Mark 
and Jeff here) can pull anything we like from it to submit to hackers.  
There's also a test case set from Yeb Havinga he used for his review.


I expected to turn at least one of those into a Postgres regression 
test.  The whole thing squealed to a stop when the regression tests 
Craig was working on were raising multiple serious questions.  I didn't 
see much sense in bundling more boring, passing tests when the ones we 
already had seemed off--and no one was sure why.


Now that Tom has given some guidance on the row locking weirdness, maybe 
it's time for me to start updating those into make check form.  The 
documentation has been in a similar holding pattern.  I have lots of 
resources to help document what does and doesn't work here to the 
quality expected in the manual.  I just need a little more confidence 
about which feature set is commit worthy.  The documentation that makes 
sense is very different if only updatable security barrier views is 
committed.


--
Greg Smith greg.sm...@crunchydatasolutions.com
Chief PostgreSQL Evangelist - http://crunchydatasolutions.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] Autonomous Transaction (WIP)

[Rajeev rastogi]

On 10 April 2014 11:18, Pavan Deolasee Wrote:
>>I could think of few  global variables like transaction properties 
>>related(i.e. read-only mode, isolation level etc). As I plan to keep 
>>transaction properties of autonomous transaction same as main transaction, so 
>>there is no need to have these global variables separately.
>>Apart from this there are global variables like with-in transaction counters, 
>>GUC, xactStartTimeStamp. I think there is no need to maintain these variables 
>>also separately. They can continue from previous value for autonomous 
>>transaction also similar to as sub->>transaction does.

>Hmm. Is that in line with what other databases do ? I would have preferred AT 
>to run like a standalone transaction without any influence of the starting 
>transaction, managing its own resources/locks/visibility/triggers etc.

To me it seems it is not very useful to keep the transaction properties 
separate except the read-only properties (though oracle does not share any 
transaction properties).

So we can have restriction that isolation and deferrable properties of main 
transaction will be inherited by autonomous transaction but read-only 
properties can be defined independently by autonomous transaction. Which looks 
to be fair restriction according to me.

In order to keep read-only properties separate, there is already infrastructure 
in PG. Inside the structure TransactionStateData, there is variable 
prevXactReadOnly (entry-time xact r/o state), which can keep the parent 
transaction read only properties and XactReadOnly can be changed to current 
transaction properties.
Moreover we can take this (transaction 
properties) as a feature enhancement also once a basic infrastructure is 
established, if acceptable to everyone.

Autonomous transaction will not share resource/lock/visibility etc with main 
transaction. This has been already taken care in WIP patch.
>>In-case of autonomous transaction, only specific global variables initialized 
>>are related to resources (similar to sub-transaction), which anyway  gets 
>>stored in current transaction state.
>>Please let me know if I am missing something or if you have some specific 
>>global variables related issue.
>No, I don't have any specific issues in mind. Mostly all such global state is 
>managed through various AtStart/AtEOX and related routines. So a careful 
>examination of all those routines will give a good idea what needs to be 
>handled. You probably will require to write
>AtATStart/AtATEOX and similar routines to manage the state at AT 
>start/commit/rollback. Sorry, I haven't looked at your WIP patch yet.

For some of the resources, I have already written AtATStart/AtATEOX kind of 
routines in WIP patch.

Comments/feedbacks/doubts are welcome.

Thanks and Regards,
Kumar Rajeev Rastogi

Re: [HACKERS] small typo about comment in xlog.c

[Heikki Linnakangas]

On 04/10/2014 07:19 AM, Tomonari Katsumata wrote:
> Hi,
> 
> I'm reading xlog.c, and I noticed a comment of
> do_pg_abort_backup is typo.
> 
> ...
> 10247 * NB: This is only for aborting a non-exclusive backup that
> doesn't write
> 10248 * backup_label. A backup started with pg_stop_backup() needs to be
> finished
> 10249 * with pg_stop_backup().
> ...
> 
> I think "A backup started with pg_stop_backup()" should be
> "A backup started with pg_start_backup()".
> 
> This is a bug about source comment, so it's not big problem.
> But I want to fix the comment.
> See attached patch.

Fixed, thanks.

- Heikki


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

[HACKERS]

[Rajeev rastogi]

On 10 April 2014 11:18, Pavan Deolasee Wrote:
>>I could think of few  global variables like transaction properties 
>>related(i.e. read-only mode, isolation level etc). As I plan to keep 
>>transaction properties of autonomous transaction same as main transaction, so 
>>there is no need to have these global variables separately.
>>Apart from this there are global variables like with-in transaction counters, 
>>GUC, xactStartTimeStamp. I think there is no need to maintain these variables 
>>also separately. They can continue from previous value for autonomous 
>>transaction also similar to as sub->>transaction does.

>Hmm. Is that in line with what other databases do ? I would have preferred AT 
>to run like a standalone transaction without any influence of the starting 
>transaction, managing its own resources/locks/visibility/triggers etc.

To me it seems it is not very useful to keep the transaction properties 
separate except the read-only properties (though oracle does not share any 
transaction properties).

So we can have restriction that isolation and deferrable properties of main 
transaction will be inherited by autonomous transaction but read-only 
properties can be defined independently by autonomous transaction. Which looks 
to be fair restriction according to me.

In order to keep read-only properties separate, there is already infrastructure 
in PG. Inside the structure TransactionStateData, there is variable 
prevXactReadOnly (entry-time xact r/o state), which can keep the parent 
transaction read only properties and XactReadOnly can be changed to current 
transaction properties.
Moreover we can take this (transaction 
properties) as a feature enhancement also once a basic infrastructure is 
established, if acceptable to everyone.

Autonomous transaction will not share resource/lock/visibility etc with main 
transaction. This has been already taken care in WIP patch.
>>In-case of autonomous transaction, only specific global variables initialized 
>>are related to resources (similar to sub-transaction), which anyway  gets 
>>stored in current transaction state.
>>Please let me know if I am missing something or if you have some specific 
>>global variables related issue.
>No, I don't have any specific issues in mind. Mostly all such global state is 
>managed through various AtStart/AtEOX and related routines. So a careful 
>examination of all those routines will give a good idea what needs to be 
>handled. You probably will require to write
>AtATStart/AtATEOX and similar routines to manage the state at AT 
>start/commit/rollback. Sorry, I haven't looked at your WIP patch yet.

For some of the resources, I have already written AtATStart/AtATEOX kind of 
routines in WIP patch.

Comments/feedbacks/doubts are welcome.

Thanks and Regards,
Kumar Rajeev Rastogi

Re: [HACKERS] Get more from indices.

[Etsuro Fujita]

(2014/04/10 0:08), Tom Lane wrote:
> Kyotaro HORIGUCHI  writes:
>> Oops! I found a bug in this patch. The previous v8 patch missed
>> the case that build_index_pathkeys() could build a partial
>> pathkeys from the index tlist.
> 
> TBH I think that's barely the tip of the iceberg of cases where this
> patch will get the wrong answer.

> Also, I don't see it doing anything to check the ordering
> of multiple index columns

I think that the following code in index_pathkeys_are_extensible() would
check the ordering:

+   if (!pathkeys_contained_in(pathkeys, root->query_pathkeys))
+   return false;

> Also, what's with the success return
> before the loop:
> 
> + if (list_length(pathkeys) == list_length(root->query_pathkeys))
> + return true;
> 
> At this point you haven't proven *anything at all* about whether the
> index columns have something to do with the query_pathkeys.

I think that the two pathkeys would be proved to be equal, if the both
conditions are satisfied.

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] WAL replay bugs

[Heikki Linnakangas]

On 04/10/2014 10:52 AM, sachin kotwal wrote:


I executed given  steps many times to produce this bug.
But still I unable to hit this bug.
I used attached scripts to produce this bug.

Can I get scripts to produce this bug?


wal_replay_bug.sh



Oh, I can't reproduce it using that script either. I must've used some 
variation of it, and posted wrong script.


The attached seems to do the trick. I changed the INSERT statements 
slightly, so that all the new rows have the same key.


Thanks for verifying this!

- Heikki


wal_replay_bug.sh
Description: application/shellscript

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

Re: [HACKERS] WAL replay bugs

[sachin kotwal]

I executed given  steps many times to produce this bug.
But still I unable to hit this bug.
I used attached scripts to produce this bug.

Can I get scripts to produce this bug?


wal_replay_bug.sh
  



-
Thanks and Regards,

Sachin Kotwal
NTT-DATA-OSS Center (Pune)
--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/WAL-replay-bugs-tp5799053p5799512.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