Re: [HACKERS] Fwd: Re: [CORE] temporal tables (SQL2011)

[Craig Ringer]

On 7 November 2016 at 05:08, Stefan Scheid  wrote:

> Hi all,
>
> are there plans to introduce temporal tables?
>
> I don't know of anybody working on them, but someone else may. Try
searching the list archives.

PostgreSQL development happens because people who want features step up and
either implement them or convince someone else to implement what they need.
The roadmap, such as it is, is "what the contributors and their various
customers want".

If this is important to you, look into what you need to do to make it
happen.

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

Re: [HACKERS] Declarative partitioning - another take

[Amit Langote]

Hi Jaime,

On 2016/11/08 2:15, Jaime Casanova wrote:
> On 28 October 2016 at 02:53, Amit Langote  
> wrote:
> 
> I started to review the functionality of this patch, so i applied all
> 9 patches. After that i found this warning, which i guess is because
> it needs a cast.

Thanks a ton for reviewing!

> After that, i tried a case that i think is important: to partition an
> already existing table. Because there is no ALTER TABL SET PARTITION
> or something similar (which i think makes sense because such a command
> would need to create the partitions and move the rows to follow the
> rule that there is no rows in a parent table).
> 
> So, what i tried was:
> 
> 1) rename original table
> 2) create a new partitioned table with old name
> 3) attach old table as a partition with bounds outside normal bounds
> and no validate
> 
> the idea is to start attaching valid partitions and delete and insert
> rows from the invalid one (is there a better way of doing that?), that
> will allow to partition a table easily.

So, step 3 creates a partition that is basically unbounded.  From there,
it seems you want to create partitions with proper bounds, moving data
into them as they are created.  It seems like a job of some redistribution
command (split partition?) which is currently unsupported.

> So far so good, until i decided points 1 to 3 should happen inside a
> transaction to make things transparent to the user.
> 
> Attached is script that shows the failure when trying it:
> 
> script 1 (failing_test_1.sql) fails the assert
> "Assert(RelationGetPartitionKey(parentRel) != NULL);" in
> transformAttachPartition() at src/backend/parser/parse_utilcmd.c:3164

Thanks for the test.  This test uncovered a bug which I have fixed in my
local repository.  Given the fix the above will work, although I see that
it's not the best way to do what you want to do.

> After that i tried the same but with an already partitioned (via
> inheritance) table and got this (i did this first without a
> transaction, doing it with a transaction will show the same failure as
> before):
> 
> script 2 (failing_test_2.sql) fails the assert
> "Assert(childrte->relkind == RELKIND_PARTITIONED_TABLE);" in
> expand_inherited_rte_internal() at
> src/backend/optimizer/prep/prepunion.c:1551

This again was an oversight/bug in the patch.  It's not supported to
combine old-style inheritance partitioning with the new partitioned
tables.  In fact, ATTACH PARTITION prevents adding a regular inheritance
parent as partition.  After fixing the bug, you would instead get this error:

alter table prueba attach partition prueba_old for values start
(unbounded, unbounded) end (2008,1) no validate;
ERROR:  cannot attach regular inheritance parent as partition

In this case, you should instead create prueba_old partition hierarchy
using the new partitioning commands and then attach the same.

> PS: i don't like the START END syntax but i don't have any ideas
> there... except, there was a reason not to use expressions (like a
> CHECK constraint?)

Expression syntax is too unrestricted.  What's to prevent users from using
completely different check constraint expressions from partition to
partition (not that any users deliberately try do that)?  With the new
partitioning syntax, you specify the partitioning columns once when
creating the partitioned parent and then specify, using partitioning
method specific syntax, the bounds for every partition of the parent.
That allows to capture the partition metadata in a form that is more
suitable to implement other features related to partitioning.

That said, I think it's always a challenge to come up with a syntax that
is universally acceptable.  For example, the recent discussion about
whether to allow inclusive/exclusive to be specified for START and END
bounds of range partitions or always assume inclusive start and exclusive
end [1].

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CA%2BTgmoaKOycHcVoed%2BF3fk-z6xUOeysQFG6HT%3Doucw76bSMHCQ%40mail.gmail.com




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

Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled

[Tsunakawa, Takayuki]

From: Michael Paquier [mailto:michael.paqu...@gmail.com]
> I just looked more deeply at your refactoring patch, and I didn't know about
> CheckTokenMembership()... The whole logic of your patch depends on it.
> That's quite a cleanup that you have here. It looks that the former
> implementation just had no knowledge of this routine or it would just have
> been used.

Yes, Microsoft recommends GetTokenMembership() because it's simpler.


> +if (IsAdministrators || IsPowerUsers)
> +return 1;
> +else
> +return 0;
> I would remove the else here.

IIRC, I sometimes saw this style of code in PostgreSQL (or in psqlODBC 
possibly...)  I'd like to leave the style choice to the committer.

Regards
Takayuki Tsunakawa


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

Re: [HACKERS] Incorrect overflow check condition for WAL segment size

[Michael Paquier]

On Tue, Nov 8, 2016 at 2:33 PM, Kuntal Ghosh  wrote:
> Either the comment is wrongly written or the check for overflow
> condition has to be fixed. Assuming the overflow check condition to be
> erroneous, I've attached a patch to fix this.

Good catch. Interesting copy-pasto from 88e9823.
-- 
Michael


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

Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled

[Michael Paquier]

On Tue, Nov 8, 2016 at 2:25 PM, Tsunakawa, Takayuki
 wrote:
> From: pgsql-hackers-ow...@postgresql.org
>> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
>> Things are this way since b15f9b08 that introduced pgwin32_is_service().
>> Still, by considering what you say, you definitely have a point that if
>> postgres is started by another service running as Local System logs are
>> going where they should not. Let's remove the check for LocalSystem but
>> still check for SE_GROUP_ENABLED.
>> So, without any refactoring work, isn't the attached patch just but fine?
>> That seems to work properly for me.
>
> Just taking a look at the patch, I'm sure it will work.
>
> Committer (Heikki?),
> v5 is refactored for HEAD, and v6 is for previous releases without 
> refactoring.  I'd like v5 to be applied to at least HEAD, as it removes a lot 
> of unnecessary code.

+if (!CheckTokenMembership(NULL, AdministratorsSid, ) ||
+!CheckTokenMembership(NULL, PowerUsersSid, ))
 {
-if ((EqualSid(AdministratorsSid, Groups->Groups[x].Sid) &&
- (Groups->Groups[x].Attributes & SE_GROUP_ENABLED)) ||
-(EqualSid(PowerUsersSid, Groups->Groups[x].Sid) &&
- (Groups->Groups[x].Attributes & SE_GROUP_ENABLED)))
-{
-success = TRUE;
-break;
-}
+log_error("could not check access token membership: error code %lu\n",
+GetLastError());
+exit(1);
 }
I just looked more deeply at your refactoring patch, and I didn't know
about CheckTokenMembership()... The whole logic of your patch depends
on it. That's quite a cleanup that you have here. It looks that the
former implementation just had no knowledge of this routine or it
would just have been used.

+if (IsAdministrators || IsPowerUsers)
+return 1;
+else
+return 0;
I would remove the else here.
-- 
Michael


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

Re: [HACKERS] Fix checkpoint skip logic on idle systems by tracking LSN progress

[Michael Paquier]

On Tue, Nov 1, 2016 at 8:31 PM, Kyotaro HORIGUCHI
 wrote:
> Could you let me struggle a bit more to avoid LWLocks in
> GetProgressRecPtr?

Be my guest :)

> I considered two alternatives for updating logic of progressAt
> more seriously. One is, as Amit suggested, replacing progressAt
> within the SpinLock section in
> ReserverXLogInsertLocation. Another is using pg_atomic_u64 for
> progressAt. The attached two patches rouhgly implement the aboves
> respectively. (But I've not tested them. The patches are to show
> the two alternatives concretely.)

Okay.

> I found that the former requires to take insertpos_lck also on
> reading. I have to admit that this is too bad. (Even I saw no
> degradation by pgbench on my poor environment. It marks 118tr/s
> by 10 threads and that doesn't seem make any stress on xlog
> logics...)

Interesting...

> For the latter, it is free from locks and doesn't reduce parallel
> degree but I'm not sure it is proper to use it there and I'm not
> sure about actual overheads.  In the worst case, it makes another
> SpinLock section for every call to pg_atmoic_* functions.

The WAL insert slots have been introduced in 9.4, and the PG atomics
in 9.5, so perhaps the first implementation of the WAL insert slots
would have used it. Still that looks quite promising. At the same time
we may be able to do something for insertingAt to make the locks held
more consistent, and just remove WALInsertLocks, even if this makes me
wonder about torn reads and about how we may break things if we rely
on something else than LW_EXCLUSIVE compared to now. To keep things
more simple I' would still favor using WALInsertLocks for this patch,
that looks more consistent, and also because there is no noticeable
difference.

> All except the above point looks good for me. Maybe it is better
> that XLOG_INCLUDE_ORIGIN stuff is in a separate patch.

I have kept that grouped bas XLOG_INCLUDE_ORIGIN is the only
XLogInsert flag present on HEAD. Could the patch be marked as "ready
for committer" then?
-- 
Michael


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

[HACKERS] Incorrect overflow check condition for WAL segment size

[Kuntal Ghosh]

Hi all,

Although we restrict the WAL segment size to 64 MB as upper limit, the
following piece of code in guc.c (line 715) seems confusing to me.

#if XLOG_SEG_SIZE < (1024*1024) || XLOG_BLCKSZ > (1024*1024*1024)
#error XLOG_SEG_SIZE must be between 1MB and 1GB
#endif

Either the comment is wrongly written or the check for overflow
condition has to be fixed. Assuming the overflow check condition to be
erroneous, I've attached a patch to fix this.

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com


wrong_wal_segment_size_upper_limit.patch
Description: application/download

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

Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> Things are this way since b15f9b08 that introduced pgwin32_is_service().
> Still, by considering what you say, you definitely have a point that if
> postgres is started by another service running as Local System logs are
> going where they should not. Let's remove the check for LocalSystem but
> still check for SE_GROUP_ENABLED.
> So, without any refactoring work, isn't the attached patch just but fine?
> That seems to work properly for me.

Just taking a look at the patch, I'm sure it will work.

Committer (Heikki?),
v5 is refactored for HEAD, and v6 is for previous releases without refactoring. 
 I'd like v5 to be applied to at least HEAD, as it removes a lot of unnecessary 
code.

Regards
Takayuki Tsunakawa



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

Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled

[Michael Paquier]

On Tue, Nov 8, 2016 at 1:36 PM, Tsunakawa, Takayuki
 wrote:
> From: pgsql-hackers-ow...@postgresql.org
>> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
>> Hm... See here:
>> http://stackoverflow.com/questions/6084547/how-to-check-whether-a-proc
>> ess-is-running-as-a-windows-service
>> And particularly this quote:
>> "No, that is not reliable because if a service is started from command line
>> for example it will not have this token. "
>
> Is there any Microsoft document that states this?  I don't think the above 
> comment is correct, because SECURITY_SERVICE_RID was present when I started 
> the service from command line with "net start".

Not that I can see of... So maybe I'm just confused by this comment as
it is added by the SCM itself, right?

Things are this way since b15f9b08 that introduced
pgwin32_is_service(). Still, by considering what you say, you
definitely have a point that if postgres is started by another service
running as Local System logs are going where they should not. Let's
remove the check for LocalSystem but still check for SE_GROUP_ENABLED.
So, without any refactoring work, isn't the attached patch just but
fine? That seems to work properly for me.
-- 
Michael
diff --git a/src/port/win32security.c b/src/port/win32security.c
index 2c9ca15..e329eb0 100644
--- a/src/port/win32security.c
+++ b/src/port/win32security.c
@@ -122,12 +122,9 @@ pgwin32_is_admin(void)
 }
 
 /*
- * We consider ourselves running as a service if one of the following is
- * true:
- *
- * 1) We are running as Local System (only used by services)
- * 2) Our token contains SECURITY_SERVICE_RID (automatically added to the
- *	  process token by the SCM when starting a service)
+ * We consider ourselves running as a service if our token contains
+ * SECURITY_SERVICE_RID, which is automatically added to the process token
+ * by the SCM when starting a service.
  *
  * Return values:
  *	 0 = Not service
@@ -147,9 +144,7 @@ pgwin32_is_service(void)
 	char	   *InfoBuffer = NULL;
 	char		errbuf[256];
 	PTOKEN_GROUPS Groups;
-	PTOKEN_USER User;
 	PSID		ServiceSid;
-	PSID		LocalSystemSid;
 	SID_IDENTIFIER_AUTHORITY NtAuthority = {SECURITY_NT_AUTHORITY};
 	UINT		x;
 
@@ -164,37 +159,6 @@ pgwin32_is_service(void)
 		return -1;
 	}
 
-	/* First check for local system */
-	if (!pgwin32_get_dynamic_tokeninfo(AccessToken, TokenUser, ,
-	   errbuf, sizeof(errbuf)))
-	{
-		fprintf(stderr, "%s", errbuf);
-		return -1;
-	}
-
-	User = (PTOKEN_USER) InfoBuffer;
-
-	if (!AllocateAndInitializeSid(, 1,
-			  SECURITY_LOCAL_SYSTEM_RID, 0, 0, 0, 0, 0, 0, 0,
-  ))
-	{
-		fprintf(stderr, "could not get SID for local system account\n");
-		CloseHandle(AccessToken);
-		return -1;
-	}
-
-	if (EqualSid(LocalSystemSid, User->User.Sid))
-	{
-		FreeSid(LocalSystemSid);
-		free(InfoBuffer);
-		CloseHandle(AccessToken);
-		_is_service = 1;
-		return _is_service;
-	}
-
-	FreeSid(LocalSystemSid);
-	free(InfoBuffer);
-
 	/* Now check for group SID */
 	if (!pgwin32_get_dynamic_tokeninfo(AccessToken, TokenGroups, ,
 	   errbuf, sizeof(errbuf)))
@@ -218,7 +182,8 @@ pgwin32_is_service(void)
 	_is_service = 0;
 	for (x = 0; x < Groups->GroupCount; x++)
 	{
-		if (EqualSid(ServiceSid, Groups->Groups[x].Sid))
+		if (EqualSid(ServiceSid, Groups->Groups[x].Sid) &&
+			(Groups->Groups[x].Attributes & SE_GROUP_ENABLED))
 		{
 			_is_service = 1;
 			break;

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

[HACKERS] SERIALIZABLE on standby servers

[Thomas Munro]

Hi hackers

Here is an experimental WIP patch to allow SERIALIZABLE READ ONLY
DEFERRABLE transactions on standby servers without serialisation
anomalies, based loosely on an old email from Kevin Grittner[1].  I'm
not sure how far this is from what he had in mind or whether I've
misunderstood something fundamental here, but I hope this can at least
serve as a starting point and we can try to get something into
Postgres 10.

The patch works by teaching the standby how to do somethings similar
to what SERIALIZABLE READ ONLY DEFERRABLE does on the primary server,
with some help from the primary server in the form of extra
information in the WAL.

The basic idea is: the standby should wait until a point in the
transaction stream where it can take a snapshot and either (1) there
were no concurrent read/write SERIALIZABLE transactions running on the
primary, or (2) the last concurrent read/write SERIALIZABLE
transaction at snapshot time has now ended without creating a
dangerous cycle with our transaction.

In case (1), the primary sometimes adds an extra
xl_xact_snapshot_safety struct to commit messages which says 'a
snapshot taken after this commit and before any future SSI commits is
safe, because there are no concurrent read/write SSI transactions at
this moment'.

In case (2), the xl_xact_snapshot_safety struct embedded in a commit
record instead says 'a snapshot taken after this commit and before any
future SSI commits is of unknown safety, because there are concurrent
transactions; I'll tell you when it has been determined; please
remember this token'.  The token (which happens to be a CSN but that
is not important) will appear in a future independent snapshot safety
message which says whether a snapshot taken at that time is safe or
unsafe.

Note that xl_xact_snapshot_safety is embedded in the commit messages
(for SSI transactions only), but also appears as its own WAL record to
report the final state of a token from an earlier commit.  So if you
do a lot of non-overlapping writable SSI transactions, you'll get just
a few extra bytes in each commit record, but overlapping transactions
will generate a stream of extra snapshot safety messages, one for each
commit involved.

In order to generate follow-up snapshot safety messages, the patch
creates  'hypothetical' transactions on the primary whenever a
writeable SSI transaction commits, so that it can figure out whether
such a transaction would conflict.  These phantom transactions are
proxies for any transaction that may be created on a standby at the
same point in the transaction stream (with respect to SSI commits) on
any standby, and survive in memory just until they are found to be
safe or unsafe.

Example of use:

T1 on primary: BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
T1 on primary: INSERT INTO foo VALUES ('x');
T2 on primary:   BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
T2 on primary:   INSERT INTO foo VALUES ('x');
T2 on primary:   COMMIT;
T3 on standby: BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE READ
ONLY DEFERRABLE;
T3 on standby: SELECT * FROM foo;
T3 on standby: <...waits...>
T1 on primary: COMMIT;
T3 on standby: <...continues...>

Not tested much and certainly has bugs and many details to sort out,
but first...  is this sound or could it be made so?  Is there a better
way?

[1] 
https://www.postgresql.org/message-id/4D3735E3022500039869%40gw.wicourts.gov

-- 
Thomas Munro
http://www.enterprisedb.com


ssi-standby-v1.patch
Description: Binary data

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

Re: [HACKERS] Re: [COMMITTERS] pgsql: Change the way that LWLocks for extensions are allocated.

[Amit Kapila]

On Wed, Nov 2, 2016 at 12:48 PM, Ashutosh Sharma  wrote:
> Hi,
>
> I have started with the review for this patch and would like to share
> some of my initial review comments that requires author's attention.
>

Thanks.

> 1) I am getting some trailing whitespace errors when trying to apply
> this patch using git apply command. Following are the error messages i
> am getting.
>
> [edb@localhost postgresql]$ git apply test_pg_stat_statements-v1.patch
> test_pg_stat_statements-v1.patch:28: trailing whitespace.
> $(MAKE) -C $(top_builddir)/src/test/regress all
> test_pg_stat_statements-v1.patch:41: space before tab in indent.
>  $(REGRESSCHECKS)
> warning: 2 lines add whitespace errors.
>

Fixed.

> 2) The test-case you have added is failing that is because queryid is
> going to be different every time you execute the test-case.
>

It shouldn't be different each time you execute test as this is a hash
code, but I agree that it is not stable and it can vary across
platforms, so removed it from test.

>
> 3) Ok. As you mentioned upthread, I do agree with the fact that we
> can't add pg_stat_statements tests for installcheck as this module
> requires shared_preload_libraries to be set. But, if there is an
> already existing installation with pg_stat_statements module loaded we
> should allow the test to run on this installation if requested
> explicitly by the user. I think we can add some target like
> 'installcheck-force' in the MakeFile that would help the end users to
> run the test on his own installation.
>

I had also thought about it while preparing patch, but I couldn't find
any clear use case.  I think it could be useful during development of
a module, but not sure if it is worth to add a target.  I think there
is no harm in adding such a target, but lets take an opinion from
committer or others as well before adding this target.  What do you
say?


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


test_pg_stat_statements-v2.patch
Description: Binary data

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

Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> Hm... See here:
> http://stackoverflow.com/questions/6084547/how-to-check-whether-a-proc
> ess-is-running-as-a-windows-service
> And particularly this quote:
> "No, that is not reliable because if a service is started from command line
> for example it will not have this token. "

Is there any Microsoft document that states this?  I don't think the above 
comment is correct, because SECURITY_SERVICE_RID was present when I started the 
service from command line with "net start".

Regards
Takayuki Tsunakawa


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

Re: [HACKERS] Parallel tuplesort (for parallel B-Tree index creation)

[Peter Geoghegan]

On Mon, Oct 24, 2016 at 6:17 PM, Peter Geoghegan  wrote:
>> * Cost model. Should probably attempt to guess final index size, and
>> derive calculation of number of workers from that. Also, I'm concerned
>> that I haven't given enough thought to the low end, where with default
>> settings most CREATE INDEX statements will use at least one parallel
>> worker.

> While I haven't made progress on any of these open items, I should
> still get a version out that applies cleanly on top of git tip --
> commit b75f467b6eec0678452fd8d7f8d306e6df3a1076 caused the patch to
> bitrot. I attach V4, which is a fairly mechanical rebase of V3, with
> no notable behavioral changes or bug fixes.

I attach V5. Changes:

* A big cost model overhaul. Workers are logarithmically scaled based
on projected final *index* size, not current heap size, as was the
case in V4. A new nbtpage.c routine is added to estimate a
not-yet-built B-Tree index's size, now called by the optimizer. This
involves getting average item width for indexed attributes from
pg_attribute for the heap relation. There are some subtleties here
with partial indexes, null_frac, etc. I also refined the cap applied
on the number of workers that limits too many workers being launched
when there isn't so much maintenance_work_mem.

The cost model is much improved now -- it is now more than just a
placeholder, at least. It doesn't do things like launch a totally
inappropriate number of workers to build a very small partial index.
Granted, those workers would still have something to do -- scan the
heap -- but not enough to justify launching so many (that is,
launching as many as would be launched for an equivalent non-partial
index).

That having been said, things are still quite fudged here, and I
struggle to find any guiding principle around doing better on average.
I think that that's because of the inherent difficulty of modeling
what's going on, but I'd be happy to be proven wrong on that. In any
case, I think it's going to be fairly common for DBAs to want to use
the storage parameter to force the use of a particular number of
parallel workers.

(See also: my remarks below on how the new bt_estimate_nblocks()
SQL-callable function can give insight into the new cost model's
decisions.)

* Overhauled leader_mergeruns() further, to make it closer to
mergeruns(). We now always rewind input tapes. This simplification
involved refining some of the assertions within logtape.c, which is
also slightly simplified.

* 2 new testing tools are added to the final commit in the patch
series (not actually proposed for commit). I've added 2 new
SQL-callable functions to contrib/pageinspect.

The 2 new testing functions are:

bt_estimate_nblocks
---

bt_estimate_nblocks() provides an easy way to see the optimizer's
projection of how large the final index will be. It returns an
estimate in blocks. Example:

mgd=# analyze;
ANALYZE
mgd=# select oid::regclass as rel,
bt_estimated_nblocks(oid),
relpages,
to_char(bt_estimated_nblocks(oid)::numeric / relpages, 'FM990.990') as
estimate_actual
from pg_class
where relkind = 'i'
order by relpages desc limit 20;

rel │
bt_estimated_nblocks │ relpages │ estimate_actual
┼──┼──┼─
 mgd.acc_accession_idx_accid│
107,091 │  106,274 │ 1.008
 mgd.acc_accession_0│
169,024 │  106,274 │ 1.590
 mgd.acc_accession_1│
169,024 │   80,382 │ 2.103
 mgd.acc_accession_idx_prefixpart   │
76,661 │   80,382 │ 0.954
 mgd.acc_accession_idx_mgitype_key  │
76,661 │   76,928 │ 0.997
 mgd.acc_accession_idx_clustered│
76,661 │   76,928 │ 0.997
 mgd.acc_accession_idx_createdby_key│
76,661 │   76,928 │ 0.997
 mgd.acc_accession_idx_numericpart  │
76,661 │   76,928 │ 0.997
 mgd.acc_accession_idx_logicaldb_key│
76,661 │   76,928 │ 0.997
 mgd.acc_accession_idx_modifiedby_key   │
76,661 │   76,928 │ 0.997
 mgd.acc_accession_pkey │
76,661 │   76,928 │ 0.997
 mgd.mgi_relationship_property_idx_propertyname_key │
74,197 │   74,462 │ 0.996
 mgd.mgi_relationship_property_idx_modifiedby_key   │
74,197 │   74,462 │ 0.996
 mgd.mgi_relationship_property_pkey │
74,197 │   74,462 │ 0.996
 mgd.mgi_relationship_property_idx_clustered│
74,197 │   74,462 │ 0.996
 mgd.mgi_relationship_property_idx_createdby_key│
74,197 │   74,462 │ 0.996
 mgd.seq_sequence_idx_clustered │
50,051 │   50,486 │ 0.991
 mgd.seq_sequence_raw_pkey  │
35,826 │   35,952 │ 0.996
 mgd.seq_sequence_raw_idx_modifiedby_key│
35,826 │   35,952 │ 0.996
 mgd.seq_source_assoc_idx_clustered │
35,822 │   35,952 │ 0.996
(20 rows)

I haven't 

Re: [HACKERS] Declarative partitioning - another take

[Amit Langote]

Hi Jaime,

On 2016/11/08 2:24, Jaime Casanova wrote:
> On 7 November 2016 at 12:15, Jaime Casanova
>  wrote:
>> On 28 October 2016 at 02:53, Amit Langote  
>> wrote:
>>>
>>> Please find attached the latest version of the patches
>>
>> Hi,
>>
>> I started to review the functionality of this patch, so i applied all
>> 9 patches. After that i found this warning, which i guess is because
>> it needs a cast.
>>
> 
> oh! i forgot the warning
> """
> partition.c: In function ‘get_qual_for_list’:
> partition.c:1159:6: warning: assignment from incompatible pointer type
>or = makeBoolExpr(OR_EXPR, list_make2(nulltest2, opexpr), -1);
>   ^
> """

This one I noticed too and have fixed.

> 
> attached a list of the warnings that my compiler give me (basically
> most are just variables that could be used uninitialized)

Thanks a lot for spotting and reporting these.  Will fix as appropriate.

Regards,
Amit




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

Re: [HACKERS] Replace use malloc() & friend by memory contexts for plperl and pltcl

[Michael Paquier]

On Tue, Nov 8, 2016 at 7:39 AM, Jim Nasby  wrote:
> On 8/31/16 2:57 AM, Michael Paquier wrote:
> Seems like a good idea, I'm guessing it slipped through the cracks. Do you
> want to add it to the next CF?

0001 has been pushed as d062245b.

> Why mark one as volatile but not the other? Based on [1] ISTM there's no need 
> to mark either as volatile?

plan_cxt is referenced in the PG_TRY block, and then modified in the
PG_CATCH block, so it seems to me that we had better mark it as
volatile to be POSIX-compliant. That's not the case of oldcontext.
-- 
Michael


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

Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)

[Haribabu Kommi]

Thanks for the patch. This shows a very good performance improvement.

I started reviewing the patch, during this process and I ran the regression
test on the WARM patch. I observed a failure in create_index test.
This may be a bug in code or expected that needs to be corrected.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Add support for SRF and returning composites to pl/tcl

[Jim Nasby]

On 11/6/16 12:15 PM, Tom Lane wrote:

I wrote:

I got the code to a state that I liked (attached), and started reviewing
the docs, and then it occurred to me to wonder why you'd chosen to use
Tcl lists to represent composite output values.  The precedent established
by input argument handling is that composites are transformed to Tcl
arrays.  So shouldn't we use an array to represent a composite result,
too?


After further nosing around I see that the return-a-tuple-as-a-list
concept is already embedded in pltcl_trigger_handler.  So the
inconsistency is already there, and it's not necessarily this patch's
job to fix it.  Still seems like we might want to allow using an array
directly rather than insisting on conversion to a list, but that's a
task for a separate patch.


My understanding is that the TCL community is of mixed feelings when it 
comes to arrays vs lists. It does seem worth adding array support though.



We should, however, make some attempt to ensure that the list-to-tuple
conversion semantics are the same in both cases.  In particular I notice
some undocumented behavior around a magic ".tupno" element.  Will see
about cleaning that up.


Hrm, I completely spaced on the fact that composite returns are 
essentially the same thing as trigger returns. ISTM we should be able to 
use the same code for both. IIRC those magic elements could end up in 
any SPI result, so that handling certainly needs to be the same.


Have you had a chance to look at this or should I?
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


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

Re: [HACKERS] Replace use malloc() & friend by memory contexts for plperl and pltcl

[Jim Nasby]

On 8/31/16 2:57 AM, Michael Paquier wrote:

Hi all,

Cleanup $subject has been raised a couple of times, like one year ago here:
https://www.postgresql.org/message-id/cab7npqrxvq+q66ufzd9wa5uaftyn4wauadbjxkfrync96kf...@mail.gmail.com
And more recently here while working on the NULL checks for malloc():
https://www.postgresql.org/message-id/CAB7nPqR7ozfCqc6C=2+ctCcnuehTZ-vTDQ8MMhY=bqyet0h...@mail.gmail.com

Attached are a set of patches to replace those memory system calls by
proper memory contexts:
- 0001 does the cleanup work for pltcl
- 0002 does this cleanup for plperl

Thoughts?


Seems like a good idea, I'm guessing it slipped through the cracks. Do 
you want to add it to the next CF?


I've looked at the pltcl patch, and it looks sane. I did have one 
question though...


+   volatile MemoryContext plan_cxt = NULL;
...
+   MemoryContext oldcontext = CurrentMemoryContext;

Why mark one as volatile but not the other? Based on [1] ISTM there's no 
need to mark either as volatile?


1: http://www.barrgroup.com/Embedded-Systems/How-To/C-Volatile-Keyword
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


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

Re: [HACKERS] Fix bug in handling of dropped columns in pltcl triggers

[Jim Nasby]

On 11/4/16 4:28 PM, Tom Lane wrote:

My proposal therefore is for SPI_fnumber to ignore (not match to)
dropped columns, and to remove any caller-side attisdropped tests that
thereby become redundant.


Yeah, SPI users certainly shouldn't need to worry about attisdropped, 
and exposing attnum doesn't seem like a good idea either.



It's possible that it'd make sense for pltcl_trigger_handler to ignore
empty-string column names in the returned list, so that the behavior
with stupid trigger functions would be a bit more forgiving; but that
is more or less independent of this patch.


I'm a bit reluctant to do that since it'd be nice to be consistent with 
regular pltcl functions returning composites. The same kind of issue 
exists with the holes in $TG_relatts; we shouldn't be exposing the 
details of attnum that way. Any code that's expecting those holes is 
going to blow up after a dump/restore anyway.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


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

Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled

[Michael Paquier]

On Tue, Nov 8, 2016 at 12:16 PM, Tsunakawa, Takayuki
 wrote:
> From: pgsql-hackers-ow...@postgresql.org
>> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
>> https://msdn.microsoft.com/ja-jp/library/windows/desktop/ms684190(v=vs
>> > .85).aspx
>>
>> That's what I looked at as well :) And this part is what caught my attention,
>> meaning that it is not used by anything else than the SCM:
>> "The LocalSystem account is a predefined local account used by the service
>> control manager."
>
> The same thing is said about other two special accounts, so they need to be 
> checked if we really believe we need to check for LocalSystem.
>
> "The LocalService account is a predefined local account used by the service 
> control manager."
> "The NetworkService account is a predefined local account used by the service 
> control manager."
>
> But, in practice, SECURITY_SERVICE_RID has turned out to be enough.

Hm... See here:
http://stackoverflow.com/questions/6084547/how-to-check-whether-a-process-is-running-as-a-windows-service
And particularly this quote:
"No, that is not reliable because if a service is started from command
line for example it will not have this token. "
-- 
Michael


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

Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> https://msdn.microsoft.com/ja-jp/library/windows/desktop/ms684190(v=vs
> > .85).aspx
> 
> That's what I looked at as well :) And this part is what caught my attention,
> meaning that it is not used by anything else than the SCM:
> "The LocalSystem account is a predefined local account used by the service
> control manager."

The same thing is said about other two special accounts, so they need to be 
checked if we really believe we need to check for LocalSystem.

"The LocalService account is a predefined local account used by the service 
control manager."
"The NetworkService account is a predefined local account used by the service 
control manager."

But, in practice, SECURITY_SERVICE_RID has turned out to be enough.


> And this implies, at least it seems to me, that trying to run Postgres as
> this user is actually not something you'd want to do.

Yes, I think people should avoid using LocalSystem for user services like 
PostgreSQL for security reasons.  But the Services applet in the Control Panel 
allows to select LocalSystem, and pg_ctl register creates a service with 
LocalSystem account when -U is omitted.

Regards
Takayuki Tsunakawa



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

Re: [HACKERS] C based plugins, clocks, locks, and configuration variables

[Craig Ringer]

On 8 November 2016 at 07:41, Clifford Hammerschmidt
 wrote:
> Hi Craig,
>
> Thanks for the pointers; I made a stab at it in:
> https://github.com/tanglebones/pg_tuid
>
> I've no idea if the shmem and lwlock code is correct, or how to test it. It
> seems to work (requires loading via the shared_preload_libraries) on osx in
> that the tuid_ calls work and produce the expected results on my lightly
> loaded development box (not really a good test of shmem or locks in that I
> doubt either are being exercised).

Since that's a public github I took the liberty of replying to the
list. Please reply to the list, not just to me.

Good on you for giving it a go.

For concurrency testing, the isolation tester tool in
src/test/isolation is quite handy. Custom pgbench scripts can also be
useful, though they're really only useful if you can detect an
anomalous situation and Assert to crash the backend in an
--enable-cassert build when there's a problem.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Remote DBA, 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] Remove the comment on the countereffectiveness of large shared_buffers on Windows

[Amit Kapila]

On Tue, Nov 8, 2016 at 5:12 AM, Jeff Janes  wrote:
> On Mon, Nov 7, 2016 at 5:55 AM, Amit Kapila  wrote:
>>
>> On Tue, Sep 20, 2016 at 8:15 AM, Tsunakawa, Takayuki
>>  wrote:
>> > I ran read-only and read-write modes of pgbench, and could not see any
>> > apparent decrease in performance when I increased shared_buffers.  The
>> > scaling factor is 200, where the database size is roughly 3GB.  I ran the
>> > benchmark on my Windows 10 PC with 6 CPU cores and 16GB of RAM.  The
>> > database and WAL is stored on the same HDD.
>> >
>> > <>
>> > @echo off
>> > for %%s in (256MB 512MB 1GB 2GB 4GB) do (
>> >   pg_ctl -w -o "-c shared_buffers=%%s" start
>> >   pgbench -c18 -j6 -T60 -S bench >> g:\b.txt 2>&1
>> >   pg_ctl -t 3600 stop
>> > )
>> >
>> > <>
>> > shared_buffers  tps
>> > 256MB  63056
>> > 512MB  63918
>> > 1GB  65520
>> > 2GB  66840
>> > 4GB  68270
>> >
>> > <>
>> > shared_buffers  tps
>> > 256MB  1138
>> > 512MB  1187
>> > 1GB  1571
>> > 2GB  1650
>> > 4GB  1598
>> >
>>
>> Isn't it somewhat strange that writes are showing big win whereas
>> reads doesn't show much win?
>
>
> I don't find that unusual, and have seen the same thing on Linux.
>
> With small shared_buffers, you are constantly throwing dirty buffers at the
> kernel in no particular order, and the kernel does a poor job of predicting
> when the same buffer will be dirtied repeatedly and only needs the final
> version of the data actually written to disk.
>

Okay and I think partially it might be because we don't have writeback
optimization (done in 9.6) for Windows.  However, still the broader
question stands that whether above data is sufficient to say that we
can recommend the settings of shared_buffers on Windows similar to
Linux?


-- 
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] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled

[Michael Paquier]

On Tue, Nov 8, 2016 at 11:36 AM, Tsunakawa, Takayuki
 wrote:
> SECURITY_SERVICE_RID
> Accounts authorized to log on as a service. This is a group identifier added 
> to the token of a process when it was logged as a service. The corresponding 
> logon type is LOGON32_LOGON_SERVICE.
>
> I saw descriptions that LocalSystem is used by the SCM, but didn't find a 
> statement that LocalSystem is used only by SCM and services.  In addition, if 
> the check for LocalSystem is really necessary, LocalService and 
> NetworkService also need to be checked.
>
> https://msdn.microsoft.com/ja-jp/library/windows/desktop/ms684190(v=vs.85).aspx

That's what I looked at as well :) And this part is what caught my
attention, meaning that it is not used by anything else than the SCM:
"The LocalSystem account is a predefined local account used by the
service control manager."
And this implies, at least it seems to me, that trying to run Postgres
as this user is actually not something you'd want to do.

> (2)
> The OP wants to explicitly run postgres.exe outside the service even when his 
> app runs as a service, so that the app can read postgres's messages from its 
> stdout/stderr.  So, he disabled SECURITY_SERVICE_RID when starting 
> postgres.exe.  His users may run his app as a service under LocalSystem.

Good question, and I don't know how this is used.
-- 
Michael


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

Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> Meh. Local System accounts are used only by services (see comments of
> pgwin32_is_service), so I'd expect pgwin32_is_service() to return true in
> this case, contrary to what your v5 is doing. v4 is doing it better I think
> at quick glance.
> Not relying on the fact that local system accounts are only used by services
> looks bad to me.

I believe v5 is correct for two reasons:


(1) 
SECURITY_SERVICE_RID is enough to check, because the process gets 
SECURITY_SERVICE_RID when it runs as a service.

https://msdn.microsoft.com/ja-jp/library/windows/desktop/aa379649(v=vs.85).aspx

SECURITY_SERVICE_RID
Accounts authorized to log on as a service. This is a group identifier added to 
the token of a process when it was logged as a service. The corresponding logon 
type is LOGON32_LOGON_SERVICE.


I saw descriptions that LocalSystem is used by the SCM, but didn't find a 
statement that LocalSystem is used only by SCM and services.  In addition, if 
the check for LocalSystem is really necessary, LocalService and NetworkService 
also need to be checked.

https://msdn.microsoft.com/ja-jp/library/windows/desktop/ms684190(v=vs.85).aspx

(Japanese article)
http://www.atmarkit.co.jp/ait/articles/0905/08/news095.html


(2)
The OP wants to explicitly run postgres.exe outside the service even when his 
app runs as a service, so that the app can read postgres's messages from its 
stdout/stderr.  So, he disabled SECURITY_SERVICE_RID when starting 
postgres.exe.  His users may run his app as a service under LocalSystem.

[Excerpt]
--
We ship PG with our own product, which may or may not be
installed as a service.  When running PG, we run postgres.exe directly via a
Tcl-based wrapper script so that we can monitor the output in real time. 

When our product is installed as a service, we use CreateRestrictedToken to
disable all admin rights as well as the SECURITY_SERVICE_RID, and use the
returned token with CreateProcessAsUser, for which we also specify
CREATE_NEW_CONSOLE.  This process then calls our wrapper script.  Inside
this wrapper, I can call GetStdHandle (via Twapi) and get valid handles for
all 3: in, out, and err.  Yet when the script calls postgres.exe, nothing is
received on the output.  As mentioned above, nothing is logged in the event
log, either.
--


Regards
Takayuki Tsunakawa

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

Re: [HACKERS] Re: [sqlsmith] FailedAssertion("!(XLogCtl->Insert.exclusiveBackup)", File: "xlog.c", Line: 10200)

[Kyotaro HORIGUCHI]

Hello,

At Sat, 5 Nov 2016 21:18:42 +0900, Michael Paquier  
wrote in 

Re: [HACKERS] Do we need use more meaningful variables to replace 0 in catalog head files?

[Michael Paquier]

On Tue, Nov 8, 2016 at 10:57 AM, Hao Lee  wrote:
> It's a tedious work to figure out these numbers real meaning. for example,
> if i want to know the value of '71'  represent what it is. I should go back
> to refer to definition of pg_class struct. It's a tedious work and it's not
> maintainable or readable.  I THINK WE SHOULD USE a meaningful variable
> instead of '71'. For Example:
>
> #define PG_TYPE_RELTYPE 71

You'd need to make genbki.pl smarter regarding the way to associate
those variables with the defined variables, greatly increasing the
amount of work it is doing as well as its maintenance (see for PGUID
handling for example). I am not saying that this is undoable, just
that the complexity may not be worth the potential readability gains.
-- 
Michael


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

Re: [HACKERS] Radix tree for character conversion

[Kyotaro HORIGUCHI]

Hello,

At Mon, 7 Nov 2016 17:19:29 +0100, Daniel Gustafsson  wrote in 
<39e295b9-7391-40b6-911d-fe852e460...@yesql.se>
> > On 07 Nov 2016, at 12:32, Daniel Gustafsson  wrote:
> > 
> >> On 04 Nov 2016, at 08:34, Kyotaro HORIGUCHI 
> >>  wrote:
> >> 
> >> I'm not sure how the discussion about this goes, these patches
> >> makes me think about coding style of Perl.
> > 
> > Some of this can absolutely be considered style and more or less down to
> > personal preference.  I haven’t seen any coding conventions for Perl so I
> > assume it’s down to consensus among the committers.
> 
> Actually, scratch that; there is of course a perltidy profile in the pgindent
> directory.  I should avoid sending email before coffee..

Hmm.  Somehow perl-mode on my Emacs is stirring with
ununderstandable indentation and I manually correct them so it is
highly probable that the style of this patch is not compatible
with the defined style. Anyway it is better that pgindent
generates smaller patch so I'll try it.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

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

[HACKERS] Do we need use more meaningful variables to replace 0 in catalog head files?

[Hao Lee]

Hi guys,
   Although, usually, we do not change the system catalog or modify the
catalog schema, or adding a new system catalog, but in these system catalog
head files, such as pg_xxx.h, i think we should use more meaningful
variables. As we known, in pg_xxx.h files, we insert some initial values
into system catalog, as following shown in pg_class.h.

DATA(insert OID = 1247 (  pg_type PGNSP 71 0 PGUID 0 0 0 0 0 0 0 f f p r 30
0 t f f f f f f t n 3 1 _null_ _null_ ));
DESCR("");
DATA(insert OID = 1249 (  pg_attribute PGNSP 75 0 PGUID 0 0 0 0 0 0 0 f f p
r 21 0 f f f f f f f t n 3 1 _null_ _null_ ));
DESCR("");

It's a tedious work to figure out these numbers real meaning. for example,
if i want to know the value of '71'  represent what it is. I should go back
to refer to definition of pg_class struct. It's a tedious work and it's not
maintainable or readable.  I THINK WE SHOULD USE a meaningful variable
instead of '71'. For Example:

#define PG_TYPE_RELTYPE 71



Regards,

Hom.

Re: [HACKERS] Radix tree for character conversion

[Kyotaro HORIGUCHI]

Hello,

At Mon, 7 Nov 2016 12:32:55 +0100, Daniel Gustafsson  wrote in 

> > On 04 Nov 2016, at 08:34, Kyotaro HORIGUCHI 
> >  wrote:
> > I'm not sure how the discussion about this goes, these patches
> > makes me think about coding style of Perl.
> 
> Some of this can absolutely be considered style and more or less down to
> personal preference.  I haven’t seen any coding conventions for Perl so I
> assume it’s down to consensus among the committers.  My rationale for these
> patches in the first place was that I perceived this thread to partly want to
> clean up the code and make it more modern Perl.
> 
> > The distinction between executable script and library is by
> > intention with an obscure basis. Existing scripts don't get less
> > modification, but library uses more restricted scopes to get rid
> > of the troubles caused by using global scopes. But I don't have a
> > clear preference on that. The TAP test scripts takes OO notations
> > but I'm not sure convutils.pl also be better to take the same
> > notation. It would be rarely edited hereafter and won't gets
> > grown any more.
> 
> I think the current convutils module is fine and converting it to OO would be
> overkill.

Agreed.

> > As far as I see the obvious bug fixes in the patchset are the
> > following,
> 
> Agreed, with some comments:
> 
> > - 0007: load_maptable fogets to close input file.
> 
> An interesting note on this is that it’s not even a bug =) Since $in is a
> scalar reference, there is no need to explicitly close() the filehandle since
> the reference counter will close it on leaving scope, but there’s no harm in
> doing it ourselves and it also makes for less confusion for anyone not 
> familiar
> with Perl internals.

Wow. I didn't know that perl has such a hidden-OO
feature. Nevertheless, implicit close is not friendly to who are
not familiar with newer perl.

Your comment led me to confirm the requirement to build PostgreSQL.

https://www.postgresql.org/docs/devel/static/install-requirements.html

| Perl 5.8 or later is needed to build from a Git checkout, or if
| you changed the input files for any of the build steps that use
| Perl scripts. If building on Windows you will need Perl in any
| case. Perl is also required to run some test suites.

So, we should assume Perl 5.8 (released in 2002!) on build
time. And actually 5.10 on RedHat 6.4, 5.16 on my
environment(ContOS 7.2), and the official doc is at 5.24. Active
perl is 5.24. According to this, we should use syntax supported
as of 5.8 and/but not obsolete until 5.24, then to follow the
latest convention. But not OO. (But I can't squeeze out a
concrete syntax set out of this requirements :( )


> > - 0010: commment for load_maptables is wrong.
> 
> There is also a fix for a typo in make_mapchecker.pl
> 
> > - 0011: hash reference is incorrectly dereferenced
> > 
> > All other fixes other than the above three seem to be styling or
> > syntax-generation issues and I don't know whether any
> > recommendation exists…
> 
> I think there are some more fixes that arent styling/syntax remaining.  I’ll 
> go
> through the patches one by one:
> 
> 0007 - While this might be considered styling/syntax, my $0.02 is that it’s 
> not
> and instead a worthwhile change.  I’ll illustrate with an example from the
> patch in question:
> 
> Using a bareword global variable in open() for the filehandle was replaced 
> with
> the three-part form in 5.6 and is now even actively discouraged from in the
> Perl documentation (and has been so since the 5.20 docs).  The problem is that
> they are global and can thus easily clash, so easily that the 0007 patch
> actually fixes one such occurrence:

That's what should be adopted in the criteria above.

  - Don't use bareword globals.
  - Use open() with separate MODE argument.

> print_radix_map() opens the file in the global filehandle OUT and passes it to
> print_radix_table() with the typeglob *OUT; print_radit_table() in turn passes
> the filehandle to print_segmented_table() which writes to the file using the
> parameter $hd, except in one case where it uses the global OUT variable 
> without
> knowing it will be the right file.  This is where the hunk below in 0007 comes
> in:
> 
> -   print OUT "$line\n";
> +   print { $$hd } "$line\n";
> 
> In this case OUT references the right file and it produces the right result,
> but it illustrates how easy it is to get wrong (which can cause very subtle
> bugs).  So, when poking at this code we might as well, IMHO, use what is today
> in Perl considered the right way to deal with filehandle references.

Thanks for the detail. Ok, I'll change the style so in the next
patch.

> Using implicit filemodes can also introduce bugs when opening filenames passed
> in from the outside as we do in UCS_to_most.pl.  Considering the use case of
> these scripts it’s obviously quite low on the list 

Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled

[Michael Paquier]

On Tue, Nov 8, 2016 at 6:47 AM, MauMau  wrote:
> As I guessed in the previous mail, both our patches cause
> pgwin32_is_service() to return 1 even when SECURITY_SERVICE_RID is
> disabled, if the service is running as a Local System.  The existing
> logic of checking for Local System should be removed.  The attached
> patch fixes this problem.

Meh. Local System accounts are used only by services (see comments of
pgwin32_is_service), so I'd expect pgwin32_is_service() to return true
in this case, contrary to what your v5 is doing. v4 is doing it better
I think at quick glance.

Here are the diffs between your v4 and v5 of this refactoring btw for
people wondering:
 /*
- * We consider ourselves running as a service if one of the following is
- * true:
- *
- * 1) We are running as Local System (only used by services)
- * 2) Our token contains SECURITY_SERVICE_RID (automatically added to the
+ * We consider ourselves running as a service if
+ * our token contains SECURITY_SERVICE_RID (automatically added to the
  *   process token by the SCM when starting a service)
  *
  * Return values:
@@ -115,39 +112,13 @@ pgwin32_is_service(void)
static int  _is_service = -1;
BOOLIsMember;
PSIDServiceSid;
-   PSIDLocalSystemSid;
SID_IDENTIFIER_AUTHORITY NtAuthority = {SECURITY_NT_AUTHORITY};

/* Only check the first time */
if (_is_service != -1)
return _is_service;

-   /* First check for Local System */
-   if (!AllocateAndInitializeSid(, 1,
- SECURITY_LOCAL_SYSTEM_RID, 0, 0, 0, 0, 0, 0, 0,
- ))
-   {
-   fprintf(stderr, "could not get SID for Local System account:
error code %lu\n",
-   GetLastError());
-   return -1;
-   }
-
-   if (!CheckTokenMembership(NULL, LocalSystemSid, ))
-   {
-   fprintf(stderr, "could not check access token membership:
error code %lu\n",
-   GetLastError());
-   FreeSid(LocalSystemSid);
-   return -1;
-   }
-   FreeSid(LocalSystemSid);
-
-   if (IsMember)
-   {
-   _is_service = 1;
-   return _is_service;
-   }
-
-   /* Next check for service group */
+   /* Check for service group membership */

Not relying on the fact that local system accounts are only used by
services looks bad to me.
-- 
Michael


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

Re: [HACKERS] Measuring replay lag

[Masahiko Sawada]

On Wed, Oct 26, 2016 at 7:34 PM, Thomas Munro
 wrote:
> Hi hackers,
>
> Here is a new version of my patch to add a replay_lag column to the
> pg_stat_replication view (originally proposed as part of a larger
> patch set for 9.6[1]), like this:

Thank you for working on this!

> postgres=# select application_name, replay_lag from pg_stat_replication;
> ┌──┬─┐
> │ application_name │   replay_lag│
> ├──┼─┤
> │ replica1 │ 00:00:00.595382 │
> │ replica2 │ 00:00:00.598448 │
> │ replica3 │ 00:00:00.541597 │
> │ replica4 │ 00:00:00.551227 │
> └──┴─┘
> (4 rows)
>
> It works by taking advantage of the { time, end-of-WAL } samples that
> sending servers already include in message headers to standbys.  That
> seems to provide a pretty good proxy for when the WAL was written, if
> you ignore messages where the LSN hasn't advanced.  The patch
> introduces a new GUC replay_lag_sample_interval, defaulting to 1s, to
> control how often the standby should record these timestamped LSNs
> into a small circular buffer.  When its recovery process eventually
> replays a timestamped LSN, the timestamp is sent back to the upstream
> server in a new reply message field.  The value visible in
> pg_stat_replication.replay_lag can then be updated by comparing with
> the current time.

replay_lag_sample_interval is 1s by default but I got 1000s by SHOW command.
postgres(1:36789)=# show replay_lag_sample_interval ;
 replay_lag_sample_interval

 1000s
(1 row)

Also, I set replay_lag_sample_interval = 500ms, I got 0 by SHOW command.
postgres(1:99850)=# select name, setting, applied from
pg_file_settings where name = 'replay_lag_sample_interval';
name| setting | applied
+-+-
 replay_lag_sample_interval | 500ms   | t
(1 row)

postgres(1:99850)=# show replay_lag_sample_interval ;
 replay_lag_sample_interval

 0
(1 row)


> Compared to the usual techniques people use to estimate replay lag,
> this approach has the following advantages:
>
> 1.  The lag is measured in time, not LSN difference.
> 2.  The lag time is computed using two observations of a single
> server's clock, so there is no clock skew.
> 3.  The lag is updated even between commits (during large data loads etc).

I agree with this approach.

> In the previous version I was effectively showing the ping time
> between the servers during idle times when the standby was fully
> caught up because there was nothing happening.  I decided that was not
> useful information and that it's more newsworthy and interesting to
> see the estimated replay lag for the most recent real replayed
> activity, so I changed that.
>
> In the last thread[1], Robert Haas wrote:
>> Well, one problem with this is that you can't put a loop inside of a
>> spinlock-protected critical section.
>
> Fixed.
>
>> In general, I think this is a pretty reasonable way of attacking this
>> problem, but I'd say it's significantly under-commented.  Where should
>> someone go to get a general overview of this mechanism?  The answer is
>> not "at place XXX within the patch".  (I think it might merit some
>> more extensive documentation, too, although I'm not exactly sure what
>> that should look like.)
>
> I have added lots of comments.
>
>> When you overflow the buffer, you could thin in out in a smarter way,
>> like by throwing away every other entry instead of the oldest one.  I
>> guess you'd need to be careful how you coded that, though, because
>> replaying an entry with a timestamp invalidates some of the saved
>> entries without formally throwing them out.
>
> Done, by overwriting the newest sample rather than the oldest if the
> buffer is full.  That seems to give pretty reasonable degradation,
> effectively lowering the sampling rate, without any complicated buffer
> or rate management code.
>
>> Conceivably, 0002 could be split into two patches, one of which
>> computes "stupid replay lag" considering only records that naturally
>> carry timestamps, and a second adding the circular buffer to handle
>> the case where much time passes without finding such a record.
>
> I contemplated this but decided that it'd be best to use ONLY samples
> from walsender headers, and never use the time stamps from commit
> records for this.  If we use times from commit records, then a
> cascading sending server will not be able to compute the difference in
> time without introducing clock skew (not to mention the difficulty of
> combining timestamps from two sources if we try to do both).  I
> figured that it's better to have value that shows a cascading
> sender->standby->cascading sender round trip time that is free of
> clock skew, than a master->cascading sender->standby->cascading sender
> incomplete round trip that includes clock skew.
>
> By 

Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled

[Michael Paquier]

On Mon, Nov 7, 2016 at 10:31 PM, MauMau  wrote:
> Yes, I tested both your patch and mine.  I used the attached pg_ctl.c.
> It adds -z option which disables SECURITY_SERVICE_RID.

Okay, so you did exactly what I did except that you wrapped with an option...

> I guess you registered the service without specifying the service
> account with -U.  Then the service runs as the Local System account,
> whence pgwin32_is_service() returns 1.

Thank you, that's as you said. I was just using the local user account
which is why I did not see the difference. And now I can. I finished
by not using your version of pg_ctl but mine still the result is the
same. Hm, now that we are two folks able to test the difference, I'd
suggest that a committer pops up and pushes the one-liner patch I sent
upthread and back-patches it.

For the refactoring, I guess that we could sort that later on, and I
promise to look at soon. The issue reported on this thread has been
here for 1 year, I am glad that someone finally came up an easy way to
test things.
-- 
Michael


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

Re: [HACKERS] pg_dump, pg_dumpall and data durability

[Michael Paquier]

On Tue, Nov 8, 2016 at 1:24 AM, Albe Laurenz  wrote:
> The patch does not apply, I had to change the hunk for
> src/include/common/file_utils.h.

Yes, the patch has rotten a bit because of f82ec32a. 5d58c07a has also
made the --noxxx option names appearing as --no-xxx.

> Also, compilation fails because function "pre_fsync_fname" cannot be
> resolved during linking. Is that a typo for "pre_fsync_fname" or is
> the patch incomplete?

A typo s/pre_fsync_fname/pre_sync_fname, and a mistake from me because
I did not compile with -DPG_FLUSH_DATA_WORKS to check this code.

v2 is attached, fixing those issues.
-- 
Michael
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 371a614..dcad095 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -815,6 +815,20 @@ PostgreSQL documentation
  
 
  
+  --no-sync
+  
+   
+By default, pg_dump will wait for all files
+to be written safely to disk.  This option causes
+pg_dump to return without waiting, which is
+faster, but means that a subsequent operating system crash can leave
+the dump corrupt.  Generally, this option is useful for testing
+but should not be used when dumping data from production installation.
+   
+  
+ 
+
+ 
   --quote-all-identifiers
   

diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml
index 97168a0..4e6839b 100644
--- a/doc/src/sgml/ref/pg_dumpall.sgml
+++ b/doc/src/sgml/ref/pg_dumpall.sgml
@@ -365,6 +365,21 @@ PostgreSQL documentation
  
 
  
+  -N
+  --no-sync
+  
+   
+By default, pg_dumpall will wait for all files
+to be written safely to disk.  This option causes
+pg_dumpall to return without waiting, which is
+faster, but means that a subsequent operating system crash can leave
+the dump corrupt.  Generally, this option is useful for testing
+but should not be used when dumping data from production installation.
+   
+  
+ 
+
+ 
   --quote-all-identifiers
   

diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index 0a28124..becbc56 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -268,7 +268,7 @@ extern Archive *OpenArchive(const char *FileSpec, const ArchiveFormat fmt);
 
 /* Create a new archive */
 extern Archive *CreateArchive(const char *FileSpec, const ArchiveFormat fmt,
-			  const int compression, ArchiveMode mode,
+			  const int compression, bool dosync, ArchiveMode mode,
 			  SetupWorkerPtr setupDumpWorker);
 
 /* The --list option */
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 0e20985..5b60da6 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -56,7 +56,8 @@ static const char *modulename = gettext_noop("archiver");
 
 
 static ArchiveHandle *_allocAH(const char *FileSpec, const ArchiveFormat fmt,
-	 const int compression, ArchiveMode mode, SetupWorkerPtr setupWorkerPtr);
+	 const int compression, bool dosync, ArchiveMode mode,
+	 SetupWorkerPtr setupWorkerPtr);
 static void _getObjectDescription(PQExpBuffer buf, TocEntry *te,
 	  ArchiveHandle *AH);
 static void _printTocEntry(ArchiveHandle *AH, TocEntry *te, bool isData, bool acl_pass);
@@ -202,10 +203,12 @@ setupRestoreWorker(Archive *AHX)
 /* Public */
 Archive *
 CreateArchive(const char *FileSpec, const ArchiveFormat fmt,
-	 const int compression, ArchiveMode mode, SetupWorkerPtr setupDumpWorker)
+			  const int compression, bool dosync, ArchiveMode mode,
+			  SetupWorkerPtr setupDumpWorker)
 
 {
-	ArchiveHandle *AH = _allocAH(FileSpec, fmt, compression, mode, setupDumpWorker);
+	ArchiveHandle *AH = _allocAH(FileSpec, fmt, compression, dosync,
+ mode, setupDumpWorker);
 
 	return (Archive *) AH;
 }
@@ -215,7 +218,7 @@ CreateArchive(const char *FileSpec, const ArchiveFormat fmt,
 Archive *
 OpenArchive(const char *FileSpec, const ArchiveFormat fmt)
 {
-	ArchiveHandle *AH = _allocAH(FileSpec, fmt, 0, archModeRead, setupRestoreWorker);
+	ArchiveHandle *AH = _allocAH(FileSpec, fmt, 0, true, archModeRead, setupRestoreWorker);
 
 	return (Archive *) AH;
 }
@@ -2223,7 +2226,8 @@ _discoverArchiveFormat(ArchiveHandle *AH)
  */
 static ArchiveHandle *
 _allocAH(const char *FileSpec, const ArchiveFormat fmt,
-	  const int compression, ArchiveMode mode, SetupWorkerPtr setupWorkerPtr)
+		 const int compression, bool dosync, ArchiveMode mode,
+		 SetupWorkerPtr setupWorkerPtr)
 {
 	ArchiveHandle *AH;
 
@@ -2277,6 +2281,7 @@ _allocAH(const char *FileSpec, const ArchiveFormat fmt,
 
 	AH->mode = mode;
 	AH->compression = compression;
+	AH->dosync = dosync;
 
 	memset(&(AH->sqlparse), 0, sizeof(AH->sqlparse));
 
diff --git a/src/bin/pg_dump/pg_backup_archiver.h b/src/bin/pg_dump/pg_backup_archiver.h
index 

Re: [HACKERS] add more NLS to bin

[Michael Paquier]

On Tue, Nov 8, 2016 at 12:13 AM, Peter Eisentraut
 wrote:
> On 11/5/16 8:03 AM, Michael Paquier wrote:
>> On Sat, Nov 5, 2016 at 9:17 AM, Peter Eisentraut
>>  wrote:
>>> > On 11/3/16 7:17 PM, Michael Paquier wrote:
 >> This patch not being complicated, so I would vote for those being
 >> addressed now so as they are not forgotten even if there is a FIXME
 >> flag added. Perhaps you don't think so, and as that's a minor issue
 >> I'll be fine with your judgement as well.
>>> >
>>> > OK, I just wrapped it in translation markers as is, which should work
>>> > well enough.
>> Agreed. I don't see anything wrong with that.
>
> committed and closed the patch

Thanks for adjusting pg_rewind as well.
-- 
Michael


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

Re: [HACKERS] Remove the comment on the countereffectiveness of large shared_buffers on Windows

[Jeff Janes]

On Mon, Nov 7, 2016 at 5:55 AM, Amit Kapila  wrote:

> On Tue, Sep 20, 2016 at 8:15 AM, Tsunakawa, Takayuki
>  wrote:
> > Hello,
> >
> >> From: pgsql-hackers-ow...@postgresql.org
> >> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Magnus
> Hagander
> >> On Wed, Aug 24, 2016 at 4:35 AM, Tsunakawa, Takayuki
> >>  wrote:
> >>   As a similar topic, I wonder whether the following still holds
> true,
> >> after many improvements on shared buffer lock contention.
> >>
> >>   https://www.postgresql.org/docs/devel/static/runtime-config-re
> >> source.html
> >>
> >>   "The useful range for shared_buffers on Windows systems
> >> is generally from 64MB to 512MB."
> >>
> >>
> >>
> >>
> >> Yes, that may very much be out of date as well. A good set of benchmarks
> >> around that would definitely be welcome.
> >
> >
> > I'd like to propose the above-mentioned comment from the manual.  The
> patch is attached.
> >
> > I ran read-only and read-write modes of pgbench, and could not see any
> apparent decrease in performance when I increased shared_buffers.  The
> scaling factor is 200, where the database size is roughly 3GB.  I ran the
> benchmark on my Windows 10 PC with 6 CPU cores and 16GB of RAM.  The
> database and WAL is stored on the same HDD.
> >
> > <>
> > @echo off
> > for %%s in (256MB 512MB 1GB 2GB 4GB) do (
> >   pg_ctl -w -o "-c shared_buffers=%%s" start
> >   pgbench -c18 -j6 -T60 -S bench >> g:\b.txt 2>&1
> >   pg_ctl -t 3600 stop
> > )
> >
> > <>
> > shared_buffers  tps
> > 256MB  63056
> > 512MB  63918
> > 1GB  65520
> > 2GB  66840
> > 4GB  68270
> >
> > <>
> > shared_buffers  tps
> > 256MB  1138
> > 512MB  1187
> > 1GB  1571
> > 2GB  1650
> > 4GB  1598
> >
>
> Isn't it somewhat strange that writes are showing big win whereas
> reads doesn't show much win?


I don't find that unusual, and have seen the same thing on Linux.

With small shared_buffers, you are constantly throwing dirty buffers at the
kernel in no particular order, and the kernel does a poor job of predicting
when the same buffer will be dirtied repeatedly and only needs the final
version of the data actually written to disk. With large shared_buffers,
you only send each dirty buffer to the kernel once per checkpoint.

Cheers,

Jeff

Re: [HACKERS] emergency outage requiring database restart

[Merlin Moncure]

On Wed, Nov 2, 2016 at 10:45 AM, Oskari Saarenmaa  wrote:
> 26.10.2016, 21:34, Andres Freund kirjoitti:
>>
>> Any chance that plsh or the script it executes does anything with the file
>> descriptors it inherits? That'd certainly one way to get into odd corruption
>> issues.
>>
>> We processor really should use O_CLOEXEC for the majority of it file
>> handles.
>
>
> Attached a patch to always use O_CLOEXEC in BasicOpenFile if we're not using
> EXEC_BACKEND.  It'd be nice to not expose all fds to most pl-languages
> either, but I guess there's no easy solution to that without forcibly
> closing all fds whenever any functions are called.

FYI, this is not my first run-in with strange behavior, on this thread
(not necessarily worth reading);
https://www.postgresql.org/message-id/CAHyXU0x5mW-SbSuUBEshzumOaN7JPUWa7Ejza68HE-KY0Nq7Kg%40mail.gmail.com

I had a similar set of starting conditions that resulted in very
strange behavior (but not data corruption AFAICT) --the problem
mysteriously disappeared when I fixed some bugs that would cause the
routine to concurrently do the same operation.  I would like to point
out that I use both pl/sh (and via it, sqsh) very highly, so these
problems are not necessarily the norm.

Regardless, it seems like you might be on to something, and I'm
inclined to patch your change, test it, and roll it out to production.
If it helps or at least narrows the problem down, we ought to give it
consideration for inclusion (unless someone else can think of a good
reason not to do that, heh!).

merlin


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

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Gilles Darold]

Le 04/11/2016 à 21:07, Karl O. Pinc a écrit :
> On Fri, 4 Nov 2016 16:58:45 +0100
> Gilles Darold  wrote:
>
>> I attached a v12 patch 
> Attached is a comment patch which improves the comment
> describing CURRENT_LOG_FILENAME.  It's been bugging me.
> I should have made this change long ago when I looked
> at all the other code comments but neglected to.
>
> The comment now matches other documentation.
>
> This applies on top of your v12 patch.

Here is the v13 of the patch, it applies your last change in comment and
some other changes:

  - I've reverted the patch removing the call to logfile_writename in
open_csvlogfile function, it is required to write log filename at
startup when log_destination is set to csvlog. I could not find a better
place right now, but will try to see if we can avoid the call to
logfile_writename().
  - Do not write current_logfiles when log_collector is activated but
log_destination doesn't contained stderr or csvlog. This was creating an
empty file that can confuse the user.
  - I've changed the comment in doc/src/sgml/storage.sgml by addind the
following sentence: "This file is present only when logging_collector is
activated and when at least one of stderr or csvlog value is present in
log_destination."


-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index adab2f8..645cbb9 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4143,6 +4143,12 @@ SELECT * FROM parent WHERE key = 2400;
  server log
 
 
+When logs are written to the file-system their paths, names, and
+types are recorded in
+the  file.  This provides
+a convenient way to find and access log content without establishing a
+database connection.
+
 
  Where To Log
 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 2e64cc4..f5bfe59 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -15443,6 +15443,19 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
   
 
   
+   pg_current_logfile()
+   text
+   primary log file name in use by the logging collector
+  
+
+  
+   pg_current_logfile(text)
+   text
+   log file name, of log in the requested format, in use by the
+   logging collector
+  
+
+  
pg_my_temp_schema()
oid
OID of session's temporary schema, or 0 if none
@@ -15660,6 +15673,34 @@ SET search_path TO schema , schema, ..
 the time when the postmaster process re-read the configuration files.)

 
+
+pg_current_logile
+   
+
+   
+Logging
+pg_current_logfile function
+   
+
+   
+pg_current_logfile returns, as text,
+the path of either the csv or stderr log file currently in use by the
+logging collector.  This is a path including the
+ directory and the log file name.
+Log collection must be active or the return value
+is NULL.  When multiple logfiles exist, each in a
+different format, pg_current_logfile called
+without arguments returns the path of the file having the first format
+found in the ordered
+list: stderr, csvlog.
+NULL is returned when no log file has any of these
+formats.  To request a specific file format supply,
+as text, either csvlog
+or stderr as the value of the optional parameter. The
+return value is NULL when the log format requested
+is not a configured .
+   
+

 pg_my_temp_schema

diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml
index fddb69b..40ac876 100644
--- a/doc/src/sgml/storage.sgml
+++ b/doc/src/sgml/storage.sgml
@@ -60,6 +60,30 @@ Item
  Subdirectory containing per-database subdirectories
 
 
+
+ 
+  
+   current_logfiles
+  
+  
+   Logging
+   current_logfiles file
+  
+  current_logfiles
+ 
+ 
+  A file recording the current log file(s) used by the syslogger and
+  its log format, stderr or csvlog. Each line
+  of the file is a space separated list of two elements: the log format and
+  the full path to the log file including the value
+  of . The log format must be present
+  in  to be listed in the file. This file
+  is present only when  is
+  activated and when at least one of stderr or
+  csvlog value is present in
+.
+
+
 
  global
  Subdirectory containing cluster-wide tables, such as
diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c
index fd62d66..6e7bdce 100644
--- a/src/backend/postmaster/syslogger.c
+++ b/src/backend/postmaster/syslogger.c
@@ -146,6 +146,7 @@ static char *logfile_getname(pg_time_t timestamp, const char *suffix);
 static void set_next_rotation_time(void);
 static void sigHupHandler(SIGNAL_ARGS);
 static void sigUsr1Handler(SIGNAL_ARGS);
+static void logfile_writename(char *filename, char *csvfilename);
 
 
 /*
@@ -348,6 +349,13 @@ SysLoggerMain(int argc, char *argv[])
 rotation_disabled = false;
 			

Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled

[MauMau]

Hi, Michael

As I guessed in the previous mail, both our patches cause
pgwin32_is_service() to return 1 even when SECURITY_SERVICE_RID is
disabled, if the service is running as a Local System.  The existing
logic of checking for Local System should be removed.  The attached
patch fixes this problem.

Regards
Takayuki Tsunakawa


win32-security-service-v5.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

[HACKERS] Fwd: Re: [CORE] temporal tables (SQL2011)

[Stefan Scheid]

Hi all,

are there plans to introduce temporal tables?

best,

Stefan



 Weitergeleitete Nachricht 
Betreff:Re: [CORE] temporal tables (SQL2011)
Datum:  Fri, 4 Nov 2016 10:27:40 -0400
Von:Peter Eisentraut 
An: Stefan Scheid 
Kopie (CC): pgsql-c...@postgresql.org



On 11/1/16 12:08 PM, Stefan Scheid wrote:
> how about implementing this feature?
> 
> Want to have a real argument to move 150 customers from mysql to
> postgresql ...
> cause they are not able or willing to use DB2 or Oracle ...

The core team does not coordinate the development effort.  Please write
to pgsql-hackers to discuss development ideas.

Re: [HACKERS] Improving executor performance

[Doug Doole]

Attached (in patch 0003) is a proof-of-concept implementing an
expression evalution framework that doesn't use recursion. Instead
ExecInitExpr2 computes a number of 'steps' necessary to compute an
expression. These steps are stored in a linear array, and executed one
after another (save boolean expressions, which can jump to later steps).
E.g. to compute colname = 1 the steps are 1) fetch var, 2) evaluate
const, 3) call function.
We've been having trouble with the performance of simple expressions in 
PLpgSQL so I started playing with this patch. (No sense re-inventing the 
wheel after all.) It was straightforward to extend to simple expressions 
and showed an immediate improvement (~10% faster on a simple test). 
Running in our full environment highlighted a few areas that I think are 
worth more investigation.


However, before I tackle that, is the posted proof-of-concept still the 
latest and greatest? If not, any chance of getting the latest?


Going forward, I'd be happy to collaborate on our efforts.

- Doug Doole
Salesforce


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

Re: [HACKERS] Let's get rid of SPI_push/SPI_pop

[Tom Lane]

I wrote:
> Pavel Stehule  writes:
>> 2016-11-07 2:16 GMT+01:00 Tom Lane :
>>> So I think we should just delete these functions and adjust SPI_connect
>>> and SPI_finish so that they just push/pop a context level unconditionally.
>>> (Which will make them simpler, not more complicated.)

>> cannot be there some performance impacts?

> Any impact will be for the better, since we're removing logic.
> I haven't gone through it in detail, but it might be as simple
> as removing _SPI_curid and code that manipulates it.

After studying spi.c more closely, I find that it can't be quite as
simple as that.  There are five functions that look at _SPI_curid
to determine where they will palloc their results:
SPI_copytuple
SPI_returntuple
SPI_modifytuple
SPI_palloc
SPI_datumTransfer
Specifically, when used in a normal SPI execution context, they
will palloc in the "savedcxt", the one that had been current when
SPI_connect was called; this behavior is used for returning values
out of a SPI-using function.  However, when not in a SPI context,
they just allocate in CurrentMemoryContext.  So their behavior is
different after SPI_push than before it.

The majority of call sites use these functions while connected to SPI,
so that it wouldn't change things if we redefined these functions
to insist on being in SPI context and always use the "savedcxt".
However, there are about half a dozen places that use SPI_modifytuple
while not connected.  Some of them never do connect at all, and
some use it after disconnecting.

I don't think that this discovery constitutes a reason not to get rid of
SPI_push/SPI_pop.  What it shows is that forgetting to call them actually
has another, subtler way of causing bugs.  If you forget, and call some
code that is using one of these functions this way and expecting to
produce a result in CurrentMemoryContext, it won't produce the result
there but in the context of the caller of your SPI-using procedure.
That probably means the memory will get leaked until you return, which
could be quite a long time (consider a long-running plpgsql function
that repeatedly calls something that acts like this).

So I think that we should still get rid of SPI_push/SPI_pop, and to that
end, redefine these five functions to throw error outside SPI context.

To simplify adapting the places that use SPI_modifytuple outside SPI
context, we could provide a function with a similar API that allocates
in CurrentMemoryContext.  I have not found any caller that really needs
a run-time determination of which memory context to use.

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] Danger of automatic connection reset in psql

[Jim Nasby]

On 11/4/16 4:04 AM, Oleksandr Shulgin wrote:

The psql process even exits with an error code 2, which might be not
that expected.  We could stop reading the file and reset connection
afterwards, but this is probably not that easy to achieve (think of
nested \i calls).


Well, if you stop reading from the file then I don't think more \i's 
will matter, no? I'd certainly like to see improvement here, because the 
difference in behavior with \i is annoying.


On the bigger question of how to better protect all these cases (cut & 
paste, etc), I think the only robust way to do that is for psql to track 
intended transaction status itself. With the amount of parsing it's 
already doing, maybe that wouldn't be that hard to add. It looks like 
this might require extra libpq calls to constantly check in on server 
status; I'm a bit surprised that result objects don't include that info.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


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

RV: [HACKERS] Compilation warning on 9.5

[Vicky Vergara]

Hello,

Posting an update to this issue (which by the way also shows up on 9.6)

Maybe this information is useful for extension developers (that have all the 
warnings flags on while developing using GNUC)


By wrapping the files as follows, any warnings generated by the postgres's 
header files are ignored.


#ifdef __GNUC__
#pragma GCC diagnostic ignored "-pedantic"
#endif

#include "postgres.h"

#ifdef __GNUC__
#pragma GCC diagnostic pop
#endif


#ifdef __GNUC__
#pragma GCC diagnostic ignored "-Wsign-conversion"
#pragma GCC diagnostic ignored "-Wunused-parameter"
#endif

#include "executor/spi.h"

#ifdef __GNUC__
#pragma GCC diagnostic pop
#pragma GCC diagnostic pop
#endif



#ifdef __GNUC__
#pragma GCC diagnostic ignored "-Wunused-parameter"
#endif

#include "funcapi.h"

#ifdef __GNUC__
#pragma GCC diagnostic pop
#endif


Vicky




De: Tom Lane 
Enviado: viernes, 12 de febrero de 2016 01:45 p. m.
Para: Vicky Vergara
Cc: pgsql-hackers@postgresql.org
Asunto: Re: [HACKERS] Compilation warning on 9.5

Vicky Vergara  writes:
> I wonder if -std=gnu99 is the correct standard to include postgres.h etc. in 
> 9.5
> because that standard (and all the flags I am using to generate pgrouting 
> code without warnings)
> catches the following catches warnings of type conversions on some postgresql 
> included files.

It's not -std=gnu99 that's causing those messages, it's -pedantic and
-Wconversion respectively.

> /usr/include/postgresql/9.5/server/c.h:298:9: warning: ISO C does not support 
> '__int128' type [-pedantic]
> /usr/include/postgresql/9.5/server/c.h:299:18: warning: ISO C does not 
> support '__int128' type [-pedantic]

We're not going to do anything about this one; certainly we won't stop
using int128 where it's available, and there isn't any other apparent way
to suppress the warning.  I doubt that -pedantic is a useful switch in
practice, and this seems to be a particularly unhelpful bit of pedantry.
Consider dropping that flag.

> /usr/include/postgresql/9.5/server/port/atomics/generic.h: In function 
> 'pg_atomic_add_fetch_u32_impl':
> /usr/include/postgresql/9.5/server/port/atomics/generic.h:238:2: warning: 
> conversion to 'uint32' from 'int32' may change the sign of the result 
> [-Wsign-conversion]

According to the gcc manual, inserting explicit casts would silence these;
is that worth doing?  I doubt anyone cares about making all our code
-Wconversion clean, but maybe making the headers clean would be worth
the trouble.

regards, tom lane

Re: [HACKERS] Specifying the log file name of pgbench -l option

[Beena Emerson]

Hello,

On Mon, Nov 7, 2016 at 8:42 PM, Masahiko Sawada 
wrote:

> On Mon, Nov 7, 2016 at 10:52 PM, Beena Emerson 
> wrote:
> > Hello Sawada-san,
> >
> > On Mon, Nov 7, 2016 at 4:47 PM, Masahiko Sawada 
> > wrote:
> >>
> >> On Wed, Nov 2, 2016 at 3:57 PM, Beena Emerson 
> >> wrote:
> >> > Hello,
> >> >
> >> > On Wed, Nov 2, 2016 at 4:16 AM, Fabien COELHO 
> >> > wrote:
> >> >>
> >> >>
> >> >> Hello Masahiko,
> >> >>
> >>  So I would suggest to:
> >>   - fix the compilation issue
> >>   - leave -l/--log as it is, i.e. use "pgbench_log" as a prefix
> >>   - add --log-prefix=... (long option only) for changing this prefix
> >> >>>
> >> >>>
> >> >>> I agree. It's better to add the separated option to specify the
> prefix
> >> >>> of log file instead of changing the existing behaviour. Attached
> >> >>> latest patch incorporated review comments.
> >> >>> Please review it.
> >> >>
> >> >>
> >> >> Patch applies, compiles and works for me.
> >> >
> >> >
> >> > It works for me as well.
> >> >
> >> >>
> >> >>
> >> >> This new option is for benchmarking, so "benchmarking_option_set"
> >> >> should
> >> >> be set to true.
> >> >>
> >> >> To simplify the code, I would suggest to initialize explicitely
> >> >> "logfile_prefix" to the default value which is then overriden when
> the
> >> >> option is set, which allows to get rid of the "prefix" variable
> later.
> >> >>
> >> >> There is no reason to change the documentation by breaking a line at
> >> >> another place if the text is not changed (where ... with 1).
> >> >>
> >> >> I would suggest to simplify a little bit the documentation:
> >> >>   "prefix of log file ..." ->
> >> >>   "default log file prefix that can be changed with ..."
> >> >>
> >> >> Otherwise the explanations seem clear enough to me. If a native
> English
> >> >> speaker could check them though, it would be nice.
> >> >
> >> >
> >> > For the explanation of the option --log-prefix, I feel it would be
> >> > better to
> >> > say "Custom prefix for transaction log file. Default is pgbench_log"
> >> >
> >> >
> >> > -   pgbench_log.nnn, where
> >> > +
> >> >
> >> > pgbench_log.<
> replaceable>nnn,
> >> > +   where pgbench_log is the prefix of log
> >> > file
> >> > that can
> >> > +   be changed by specifying --log-prefix, and where
> >> >
> >> > It could just say "the default prefix pgbench_log can be changed with
> >> > option
> >> > --log-prefix, and " we need not use where again.
> >> >
> >> > Also the error message is made similar to that of aggregate-interval
> but
> >> > I
> >> > think the one in sampling-rate is better:
> >> >
> >> > $ pgbench --log-prefix=chk -t 20
> >> > log file prefix (--log-prefix) is allowed only when actually logging
> >> > transactions
> >> >
> >> > pgbench  --sampling-rate=1 -t 20
> >> > log sampling (--sampling-rate) is allowed only when logging
> transactions
> >> > (-l)
> >> >
> >>
> >> Thank you for reviewing this patch!
> >>
> >> Attached latest patch incorporated comments.
> >> Please review it.
> >>
> >
> > Thank you for updating the patch.
> >
> > I note that the current changes, break the behaviour when we do not use
> -l
> > option.
> >
> > Since the log_prefix variable is set to default value, the check " if
> > (!use_log && logfile_prefix)"  always returns true. This throws error
> even
> > when we have not used the -l and --log-prefix option as shown below.
> >
> > $ pgbench -T 50
> > log file prefix (--log-prefix) is allowed only when logging transactions
> > (-l)
> >
>
> Thanks.
> Attached latest patch.
> Please review it.
>

Thank you for the update.

I checked. It works as expected. I have no more comments.
If its okay with Fabien, we can mark it ready for committer.

Thanks,

Beena Emerson

Have a Great Day!

Re: [HACKERS] Unsafe use of relation->rd_options without checking its type

[Tom Lane]

I wrote:
> Now that I've seen this I wonder which other uses of rd_options are
> potentially broken.  RelationIsUsedAsCatalogTable() is hardly the
> only macro that is assuming with little justification that it's
> applied to the right kind of reloptions.
> 
> We could band-aid this by having the RelationIsUsedAsCatalogTable()
> macro check relation->relkind, ...

I've pushed a hack along those lines, so that we could fix the reported
problem in the back branches.

> ... but I'm inclined to think that the
> right fix going forward is to insist that StdRdOptions, ViewOptions,
> and the other in-memory representations of reloptions ought to be
> self-identifying somehow.  We could add a field to them similar to
> nodeTag, but one of the things that was envisioned to begin with
> is relation types that have StdRdOptions as the first part of a
> larger struct.  I'm not sure how to make that work with a tag.

After a bit of thought, I propose the following conventions:

1. All decoded reloptions structs (anything that Relation.rd_options
could point to) shall embed these common header fields:

typedef struct BaseRdOptions
{
int32   vl_len_;/* varlena header (do not touch directly!) */
int options_id; /* code identifying specific reloptions */
/* useful data follows */
} BaseRdOptions;

We'll keep the rule that these structs have a varlena length word, since
having the struct size stored that way allows easy copying with datumCopy.

2. Basic reloptions structs that share no payload fields with anything
else will be assigned options_id values in the range 1..255.

3. A struct type that wants to extend, say, StdRdOptions with some extra
fields would use an options_id that has StdRdOptions's code in its low
order byte and a more specific identity in the next higher byte.
Similarly, one could extend it again with some identity bits placed in the
third byte.  There could be up to three levels of nesting before we run
out of space in the ID word, and that seems like probably enough.

4. So if you want to test whether a particular options struct has
StdRdOptions fields, you would need to execute a test like
"(rel->rd_options->options_id & 0xFF) == STDRDOPTIONS_ID".
This could and should be embedded in a standard test macro, of course.

5. The end goal would be to have all functions/macros that access
rd_options include explicit checks that the data is what they expect,
eg instead of naively doing this:

#define RelationGetFillFactor(relation, defaultff) ((relation)->rd_options 
?  ((StdRdOptions *) (relation)->rd_options)->fillfactor : (defaultff))

we should do this:

#define RelationGetFillFactor(relation, defaultff) ((relation)->rd_options 
&& IsStdRdOptions((relation)->rd_options) ?  ((StdRdOptions *) 
(relation)->rd_options)->fillfactor : (defaultff))


Unless somebody has an objection or better idea, I'll push forward
with making this happen in HEAD.

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] Declarative partitioning - another take

[Jaime Casanova]

On 7 November 2016 at 12:15, Jaime Casanova
 wrote:
> On 28 October 2016 at 02:53, Amit Langote  
> wrote:
>>
>> Please find attached the latest version of the patches
>
> Hi,
>
> I started to review the functionality of this patch, so i applied all
> 9 patches. After that i found this warning, which i guess is because
> it needs a cast.
>

oh! i forgot the warning
"""
partition.c: In function ‘get_qual_for_list’:
partition.c:1159:6: warning: assignment from incompatible pointer type
   or = makeBoolExpr(OR_EXPR, list_make2(nulltest2, opexpr), -1);
  ^
"""

attached a list of the warnings that my compiler give me (basically
most are just variables that could be used uninitialized)

-- 
Jaime Casanova  www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
partition.c: In function ‘get_qual_for_list’:
partition.c:1159:6: warning: assignment from incompatible pointer type
   or = makeBoolExpr(OR_EXPR, list_make2(nulltest2, opexpr), -1);
  ^
partition.c: In function ‘RelationBuildPartitionDesc’:
partition.c:250:8: warning: ‘num_distinct_bounds’ may be used uninitialized in 
this function [-Wmaybe-uninitialized]
  int   num_distinct_bounds;
^
partition.c:587:9: warning: ‘distinct_bounds’ may be used uninitialized in this 
function [-Wmaybe-uninitialized]
 copy_range_bound(key, distinct_bounds[i]);
 ^
partition.c:536:5: warning: ‘all_values_count’ may be used uninitialized in 
this function [-Wmaybe-uninitialized]
 for (i = 0; i < all_values_count; i++)
 ^
partition.c:243:23: warning: ‘all_values’ may be used uninitialized in this 
function [-Wmaybe-uninitialized]
  PartitionListValue **all_values;
   ^
partition.c:619:35: warning: ‘oids’ may be used uninitialized in this function 
[-Wmaybe-uninitialized]
result->oids[mapping[i]] = oids[i];
   ^
partition.c: In function ‘get_qual_from_partbound’:
partition.c:973:10: warning: ‘my_qual’ may be used uninitialized in this 
function [-Wmaybe-uninitialized]
  my_qual = (List *) map_variable_attnos((Node *) my_qual,
  ^
partition.c: In function ‘get_partition_for_tuple’:
partition.c:1734:8: warning: ‘index’ may be used uninitialized in this function 
[-Wmaybe-uninitialized]
 if (node->index > index)
^

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

Re: [HACKERS] Declarative partitioning - another take

[Jaime Casanova]

On 28 October 2016 at 02:53, Amit Langote  wrote:
>
> Please find attached the latest version of the patches

Hi,

I started to review the functionality of this patch, so i applied all
9 patches. After that i found this warning, which i guess is because
it needs a cast.

After that, i tried a case that i think is important: to partition an
already existing table. Because there is no ALTER TABL SET PARTITION
or something similar (which i think makes sense because such a command
would need to create the partitions and move the rows to follow the
rule that there is no rows in a parent table).

So, what i tried was:

1) rename original table
2) create a new partitioned table with old name
3) attach old table as a partition with bounds outside normal bounds
and no validate

the idea is to start attaching valid partitions and delete and insert
rows from the invalid one (is there a better way of doing that?), that
will allow to partition a table easily.
So far so good, until i decided points 1 to 3 should happen inside a
transaction to make things transparent to the user.

Attached is script that shows the failure when trying it:

script 1 (failing_test_1.sql) fails the assert
"Assert(RelationGetPartitionKey(parentRel) != NULL);" in
transformAttachPartition() at src/backend/parser/parse_utilcmd.c:3164

After that i tried the same but with an already partitioned (via
inheritance) table and got this (i did this first without a
transaction, doing it with a transaction will show the same failure as
before):

script 2 (failing_test_2.sql) fails the assert
"Assert(childrte->relkind == RELKIND_PARTITIONED_TABLE);" in
expand_inherited_rte_internal() at
src/backend/optimizer/prep/prepunion.c:1551

PS: i don't like the START END syntax but i don't have any ideas
there... except, there was a reason not to use expressions (like a
CHECK constraint?)

-- 
Jaime Casanova  www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


failing_test_1.sql
Description: application/sql


failing_test_2.sql
Description: application/sql

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

Re: [HACKERS] pg_dump, pg_dumpall and data durability

[Albe Laurenz]

Michael Paquier wrote:
>> In my quest of making the backup tools more compliant to data
>> durability, here is a thread for pg_dump and pg_dumpall.
> 
> Okay, here is a patch doing the above. I have added a new --nosync
> option to pg_dump and pg_dumpall to switch to the pre-10 behavior. I
> have arrived at the conclusion that it is better not to touch at
> _EndData and _EndBlob, and just issue the fsync in CloseArchive when
> all the write operations are done. In the case of the directory
> format, the fsync is done on all the entries recursively. This makes
> as well the patch more simple. The regression tests calling pg_dump
> don't use --nosync yet in this patch, that's a move that could be done
> afterwards.

The patch does not apply, I had to change the hunk for
src/include/common/file_utils.h.

Also, compilation fails because function "pre_fsync_fname" cannot be
resolved during linking. Is that a typo for "pre_fsync_fname" or is
the patch incomplete?

Yours,
Laurenz Albe

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

Re: [HACKERS] Radix tree for character conversion

[Daniel Gustafsson]

> On 07 Nov 2016, at 12:32, Daniel Gustafsson  wrote:
> 
>> On 04 Nov 2016, at 08:34, Kyotaro HORIGUCHI 
>>  wrote:
>> 
>> I'm not sure how the discussion about this goes, these patches
>> makes me think about coding style of Perl.
> 
> Some of this can absolutely be considered style and more or less down to
> personal preference.  I haven’t seen any coding conventions for Perl so I
> assume it’s down to consensus among the committers.

Actually, scratch that; there is of course a perltidy profile in the pgindent
directory.  I should avoid sending email before coffee..

cheers ./daniel

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

Re: [HACKERS] Detect supported SET parameters when pg_restore is run

[Peter Eisentraut]

On 9/27/16 6:57 PM, Vitaly Burovoy wrote:
> On 9/27/16, Vitaly Burovoy  wrote:
>> On 9/27/16, Tom Lane  wrote:
>>> (The other thing I'd want here is a --target-version option so that
>>> you could get the same output alterations in pg_dump or pg_restore to
>>> text.  Otherwise it's nigh undebuggable, and certainly much harder
>>> to test than it needs to be.)
>>
>> I thought that way. I'm ready to introduce that parameter, but again,
>> I see now it will influence only SET parameters. Does it worth it?
> 
> The only reason I have not implemented it was attempt to avoid users
> being confused who could think that result of pg_dump (we need it
> there for the plain text output) or pg_restore can be converted for
> target version to be restored without new features (but now it is
> wrong).

I'm in favor of making this work.  But I also agree with the earlier
commenters that we shouldn't overload remoteVersion to mean sometimes
source and sometimes target version.  It would probably be enough for
now to leave remoteVersion to mean source version, introduce a new field
targetVersion, default that to remoteVersion in plain text format, and
set it to the actual remote version when restoring directly to a
database.  It will need a bit of work to tie it all together, but it
shouldn't be too difficult.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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] Let's get rid of SPI_push/SPI_pop

[Pavel Stehule]

2016-11-07 15:47 GMT+01:00 Tom Lane :

> Pavel Stehule  writes:
> > 2016-11-07 2:16 GMT+01:00 Tom Lane :
> >> So I think we should just delete these functions and adjust SPI_connect
> >> and SPI_finish so that they just push/pop a context level
> unconditionally.
> >> (Which will make them simpler, not more complicated.)
>
> > cannot be there some performance impacts?
>
> Any impact will be for the better, since we're removing logic.
> I haven't gone through it in detail, but it might be as simple
> as removing _SPI_curid and code that manipulates it.
>

ok

Pavel


>
> regards, tom lane
>

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Tom Lane]

Albe Laurenz  writes:
> Tom Lane wrote:
>> I'm inclined to give this up as a bad job and go back to the
>> previous state.  We have a solution that works and doesn't
>> produce warnings; third-party authors who don't want to use it
>> are on their own.

> I think you are right.

Done.  We can always un-undo it if somebody has another idea.

regards, tom lane


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

Re: [HACKERS] Quorum commit for multiple synchronous replication.

[Masahiko Sawada]

On Tue, Oct 25, 2016 at 10:35 PM, Michael Paquier
 wrote:
> On Mon, Oct 17, 2016 at 4:00 PM, Masahiko Sawada  
> wrote:
>> Attached latest patch.
>> Please review it.
>
> Okay, so let's move on with this patch...

Thank you for reviewing this patch.

> + 
> + The keyword ANY is omissible, but note that there is
> + not compatibility between PostgreSQL version 10 and
> + 9.6 or before. For example, 1 (s1, s2) is the same as 
> the
> + configuration with FIRST and  class="parameter">
> + num_sync equal to 1 in PostgreSQL 9.6
> + or before.  On the other hand, It's the same as the configuration 
> with
> + ANY and  class="parameter">num_sync equal to
> + 1 in PostgreSQL 10 or later.
> +
> This paragraph could be reworded:
> If FIRST or ANY are not specified, this parameter behaves as ANY. Note
> that this grammar is incompatible with PostgreSQL 9.6, where no
> keyword specified is equivalent as if FIRST was specified.
> In short, there is no real need to specify num_sync as this behavior
> does not have changed, as well as it is not necessary to mention
> pre-9.6 versions as the multi-sync grammar has been added in 9.6.

Fixed.

> -Specifying more than one standby name can allow very high 
> availability.
> Why removing this sentence?
>
> +The keyword ANY, coupeld with an interger number N,
> s/coupeld/coupled/ and s/interger/integer/, for a double hit in one
> line, still...
>
> +The keyword ANY, coupeld with an interger number N,
> +chooses N standbys in a set of standbys with the same, lowest,
> +priority and makes transaction commit when WAL records are received
> +those N standbys.
> This could be reworded more simply, for example: The keyword ANY,
> coupled with an integer number N, makes transaction commits wait until
> WAL records are received from N connected standbys among those defined
> in the list of synchronous_standby_names.
>
> +s2 and s3 wil be considered as synchronous 
> standby
> s/wil/will/
>
> +  when standby is considered as a condidate of quorum commit.
> s/condidate/candidate/
>
> [... stopping here ...] Please be more careful with the documentation
> and comment grammar. There are other things in the patch..

I fix some typo as much as I found.

> A bunch of comments at the top of syncrep.c need to be updated.
>
> +extern List *SyncRepGetSyncStandbysPriority(bool *am_sync);
> +extern List *SyncRepGetSyncStandbysQuorum(bool *am_sync);
> Those two should be static routines in syncrep.c, let's keep the whole
> logic about quorum and higher-priority definition only there and not
> bother the callers of them about that.

Fixed.

> +   if (SyncRepConfig->sync_method == SYNC_REP_PRIORITY)
> +   return SyncRepGetSyncStandbysPriority(am_sync);
> +   else /* SYNC_REP_QUORUM */
> +   return SyncRepGetSyncStandbysQuorum(am_sync);
> Both routines share the same logic to detect if a WAL sender can be
> selected as a candidate for sync evaluation or not, still per the
> selection they do I agree that it is better to keep them as separate.
>
> +   /* In quroum method, all sync standby priorities are always 1 */
> +   if (found && SyncRepConfig->sync_method == SYNC_REP_QUORUM)
> +   priority = 1;
> Honestly I don't understand why you are enforcing that. Priority can
> be important for users willing to switch from ANY to FIRST to have a
> look immediately at what are the standbys that would become sync or
> potential.

I thought that since all standbys appearing in s_s_names list are
treated equally in quorum method, these standbys should have same
priority.
If these standby have different sync_priority, it looks like that
master server replicates to standby server based on priority.

> else if (list_member_int(sync_standbys, i))
> -   values[7] = CStringGetTextDatum("sync");
> +   values[7] = SyncRepConfig->sync_method == SYNC_REP_PRIORITY ?
> +   CStringGetTextDatum("sync") : 
> CStringGetTextDatum("quorum");
> The comment at the top of this code block needs to be refreshed.

Fixed.

> The representation given to the user in pg_stat_replication is not
> enough IMO. For example, imagine a cluster with 4 standbys:
> =# select application_name, sync_priority, sync_state from 
> pg_stat_replication ;
>  application_name | sync_priority | sync_state
> --+---+
>  node_5433| 0 | async
>  node_5434| 0 | async
>  node_5435| 0 | async
>  node_5436| 0 | async
> (4 rows)
>
> If FIRST N is used, is it easy for the user to understand what are the
> nodes in sync:
> =# alter system set synchronous_standby_names = 'FIRST 2 (node_5433,
> node_5434, node_5435)';
> ALTER SYSTEM
> =# select pg_reload_conf();
>  pg_reload_conf
> 
>  t
> (1 

Re: [HACKERS] add more NLS to bin

[Peter Eisentraut]

On 11/5/16 8:03 AM, Michael Paquier wrote:
> On Sat, Nov 5, 2016 at 9:17 AM, Peter Eisentraut
>  wrote:
>> > On 11/3/16 7:17 PM, Michael Paquier wrote:
>>> >> This patch not being complicated, so I would vote for those being
>>> >> addressed now so as they are not forgotten even if there is a FIXME
>>> >> flag added. Perhaps you don't think so, and as that's a minor issue
>>> >> I'll be fine with your judgement as well.
>> >
>> > OK, I just wrapped it in translation markers as is, which should work
>> > well enough.
> Agreed. I don't see anything wrong with that.

committed and closed the patch

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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] Specifying the log file name of pgbench -l option

[Masahiko Sawada]

On Mon, Nov 7, 2016 at 10:52 PM, Beena Emerson  wrote:
> Hello Sawada-san,
>
> On Mon, Nov 7, 2016 at 4:47 PM, Masahiko Sawada 
> wrote:
>>
>> On Wed, Nov 2, 2016 at 3:57 PM, Beena Emerson 
>> wrote:
>> > Hello,
>> >
>> > On Wed, Nov 2, 2016 at 4:16 AM, Fabien COELHO 
>> > wrote:
>> >>
>> >>
>> >> Hello Masahiko,
>> >>
>>  So I would suggest to:
>>   - fix the compilation issue
>>   - leave -l/--log as it is, i.e. use "pgbench_log" as a prefix
>>   - add --log-prefix=... (long option only) for changing this prefix
>> >>>
>> >>>
>> >>> I agree. It's better to add the separated option to specify the prefix
>> >>> of log file instead of changing the existing behaviour. Attached
>> >>> latest patch incorporated review comments.
>> >>> Please review it.
>> >>
>> >>
>> >> Patch applies, compiles and works for me.
>> >
>> >
>> > It works for me as well.
>> >
>> >>
>> >>
>> >> This new option is for benchmarking, so "benchmarking_option_set"
>> >> should
>> >> be set to true.
>> >>
>> >> To simplify the code, I would suggest to initialize explicitely
>> >> "logfile_prefix" to the default value which is then overriden when the
>> >> option is set, which allows to get rid of the "prefix" variable later.
>> >>
>> >> There is no reason to change the documentation by breaking a line at
>> >> another place if the text is not changed (where ... with 1).
>> >>
>> >> I would suggest to simplify a little bit the documentation:
>> >>   "prefix of log file ..." ->
>> >>   "default log file prefix that can be changed with ..."
>> >>
>> >> Otherwise the explanations seem clear enough to me. If a native English
>> >> speaker could check them though, it would be nice.
>> >
>> >
>> > For the explanation of the option --log-prefix, I feel it would be
>> > better to
>> > say "Custom prefix for transaction log file. Default is pgbench_log"
>> >
>> >
>> > -   pgbench_log.nnn, where
>> > +
>> >
>> > pgbench_log.nnn,
>> > +   where pgbench_log is the prefix of log
>> > file
>> > that can
>> > +   be changed by specifying --log-prefix, and where
>> >
>> > It could just say "the default prefix pgbench_log can be changed with
>> > option
>> > --log-prefix, and " we need not use where again.
>> >
>> > Also the error message is made similar to that of aggregate-interval but
>> > I
>> > think the one in sampling-rate is better:
>> >
>> > $ pgbench --log-prefix=chk -t 20
>> > log file prefix (--log-prefix) is allowed only when actually logging
>> > transactions
>> >
>> > pgbench  --sampling-rate=1 -t 20
>> > log sampling (--sampling-rate) is allowed only when logging transactions
>> > (-l)
>> >
>>
>> Thank you for reviewing this patch!
>>
>> Attached latest patch incorporated comments.
>> Please review it.
>>
>
> Thank you for updating the patch.
>
> I note that the current changes, break the behaviour when we do not use -l
> option.
>
> Since the log_prefix variable is set to default value, the check " if
> (!use_log && logfile_prefix)"  always returns true. This throws error even
> when we have not used the -l and --log-prefix option as shown below.
>
> $ pgbench -T 50
> log file prefix (--log-prefix) is allowed only when logging transactions
> (-l)
>

Thanks.
Attached latest patch.
Please review it.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 285608d..a6fe183 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -614,6 +614,15 @@ pgbench  options  dbname
   
  
 
+ 
+  --log-prefix=prefix
+  
+   
+Custom prefix of transaction log file. Default is pgbench_log.
+   
+  
+ 
+
 

 
@@ -1121,13 +1130,14 @@ END;
With the -l option but without the --aggregate-interval,
pgbench writes the time taken by each transaction
to a log file.  The log file will be named
-   pgbench_log.nnn, where
-   nnn is the PID of the pgbench process.
-   If the -j option is 2 or higher, creating multiple worker
-   threads, each will have its own log file. The first worker will use the
-   same name for its log file as in the standard single worker case.
+   pgbench_log.nnn,
+   where the default prefix pgbench_log can be changed with option
+   --log-prefix, and nnn is the PID of the
+   pgbench process. If the -j option is 2 or higher,
+   creating multiple worker threads, each will have its own log file. The first worker will
+   use the same name for its log file as in the standard single worker case.
The additional log files for the other workers will be named
-   pgbench_log.nnn.mmm,
+   pgbench_log.nnn.mmm,
where mmm is a sequential number for each worker starting
with 1.
   
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index d44cfda..318b132 100644
--- a/src/bin/pgbench/pgbench.c
+++ 

[HACKERS] Re: [BUGS] BUG #14350: VIEW with INSTEAD OF INSERT TRIGGER and COPY. Missing feature or working as designed.

[Dilip Kumar]

On Fri, Nov 4, 2016 at 7:14 AM, Haribabu Kommi  wrote:
>> + if (cstate->rel->rd_rel->relkind != RELKIND_RELATION
>> + && (!cstate->rel->trigdesc ||
>> + !cstate->rel->trigdesc->trig_insert_instead_row))
>
>
> changed.
>
>>
>> Meanwhile I will test it and give the feedback.
>
>
> Thanks.
>
> Updated patch is attached with added regression tests.

I have done with review and test, patch looks fine to me.
moved to "Ready for Committer"

-- 
Regards,
Dilip Kumar
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] [COMMITTERS] pgsql: pg_xlogdump: Add NLS

[Peter Eisentraut]

On 11/5/16 9:19 AM, Alvaro Herrera wrote:
> Peter Eisentraut wrote:
>> pg_xlogdump [etc]: Add NLS
> 
> So how large are the new .po files?

pg_archivecleanup/po/pg_archivecleanup.pot: 0 translated messages, 25
untranslated messages.
pg_test_fsync/po/pg_test_fsync.pot: 0 translated messages, 28
untranslated messages.
pg_test_timing/po/pg_test_timing.pot: 0 translated messages, 12
untranslated messages.
pg_upgrade/po/pg_upgrade.pot: 0 translated messages, 235 untranslated
messages.
pg_xlogdump/po/pg_xlogdump.pot: 0 translated messages, 41 untranslated
messages.
pgbench/po/pgbench.pot: 0 translated messages, 167 untranslated messages.


-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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] Let's get rid of SPI_push/SPI_pop

[Tom Lane]

Pavel Stehule  writes:
> 2016-11-07 2:16 GMT+01:00 Tom Lane :
>> So I think we should just delete these functions and adjust SPI_connect
>> and SPI_finish so that they just push/pop a context level unconditionally.
>> (Which will make them simpler, not more complicated.)

> cannot be there some performance impacts?

Any impact will be for the better, since we're removing logic.
I haven't gone through it in detail, but it might be as simple
as removing _SPI_curid and code that manipulates 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] Streaming basebackups vs pg_stat_tmp

[Magnus Hagander]

On Sat, Oct 29, 2016 at 4:12 PM, Michael Paquier 
wrote:

> On Fri, Oct 28, 2016 at 9:57 PM, David Steele  wrote:
> > On 10/28/16 3:49 PM, Magnus Hagander wrote:
> > The change from 10 to 11 increases the tests that are skipped on Windows,
> > which is necessary because one extra symlink test is added.
> >
> > I think you need:
> >
> > [...]
> >
> > The rest of the tests are for exclusions.
>
> Indeed, giving the attached for REL9_6_STABLE. You could as well have
> a test for pg_stat_tmp but honestly that's not worth it. One thing I
> have noticed is that the patch does not use _tarWriteDir() for
> pg_xlog. I think it should even if that's not addressing directly a
> bug...
>


Applied and backported, thanks. Backported to 9.4, as this is where that
exclusion code appeared.

I did not backport the tests, as we don't have the $node stuff available in
9.5 and earlier.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] add more NLS to bin

[Peter Eisentraut]

On 11/5/16 8:03 AM, Michael Paquier wrote:
>> > Yeah that was wrong anyway.  The previously existing translation markers
>> > were wrong.  We want to translate the fmt, not the formatted message.
> Does using one way or the other actually change something? Because
> pg_rewind/logging.c is not marking fmt but the message, the opposite
> of your v2 patch. One way or the other, having consistency for both
> would be nice.

Consider the following for example:

pg_log(PG_DEBUG, "fetched file \"%s\", length %d\n", filename, len);

If you pass fmt to gettext(), then it will look up the string that you
see in the code.  If you pass the formatted message to gettext(), then
it will look up something like

"fetched file \"foo\", length 12\n"

which it will not find, unless you provide translations for all
combinations of file names and lengths.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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] Remove the comment on the countereffectiveness of large shared_buffers on Windows

[Amit Kapila]

On Tue, Sep 20, 2016 at 8:15 AM, Tsunakawa, Takayuki
 wrote:
> Hello,
>
>> From: pgsql-hackers-ow...@postgresql.org
>> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Magnus Hagander
>> On Wed, Aug 24, 2016 at 4:35 AM, Tsunakawa, Takayuki
>>  wrote:
>>   As a similar topic, I wonder whether the following still holds true,
>> after many improvements on shared buffer lock contention.
>>
>>   https://www.postgresql.org/docs/devel/static/runtime-config-re
>> source.html
>>
>>   "The useful range for shared_buffers on Windows systems
>> is generally from 64MB to 512MB."
>>
>>
>>
>>
>> Yes, that may very much be out of date as well. A good set of benchmarks
>> around that would definitely be welcome.
>
>
> I'd like to propose the above-mentioned comment from the manual.  The patch 
> is attached.
>
> I ran read-only and read-write modes of pgbench, and could not see any 
> apparent decrease in performance when I increased shared_buffers.  The 
> scaling factor is 200, where the database size is roughly 3GB.  I ran the 
> benchmark on my Windows 10 PC with 6 CPU cores and 16GB of RAM.  The database 
> and WAL is stored on the same HDD.
>
> <>
> @echo off
> for %%s in (256MB 512MB 1GB 2GB 4GB) do (
>   pg_ctl -w -o "-c shared_buffers=%%s" start
>   pgbench -c18 -j6 -T60 -S bench >> g:\b.txt 2>&1
>   pg_ctl -t 3600 stop
> )
>
> <>
> shared_buffers  tps
> 256MB  63056
> 512MB  63918
> 1GB  65520
> 2GB  66840
> 4GB  68270
>
> <>
> shared_buffers  tps
> 256MB  1138
> 512MB  1187
> 1GB  1571
> 2GB  1650
> 4GB  1598
>

Isn't it somewhat strange that writes are showing big win whereas
reads doesn't show much win?  Can you once do some longer read-write
tests to see if we get consistent data and take the median-of-three
runs data?  Above benchmarks doesn't seem to indicate that increase in
shared buffers upto 25% of RAM shows significant win on Windows
systems, because the performance of writes increase upto 2GB and then
starts decreasing.


-- 
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] Specifying the log file name of pgbench -l option

[Beena Emerson]

Hello Sawada-san,

On Mon, Nov 7, 2016 at 4:47 PM, Masahiko Sawada 
wrote:

> On Wed, Nov 2, 2016 at 3:57 PM, Beena Emerson 
> wrote:
> > Hello,
> >
> > On Wed, Nov 2, 2016 at 4:16 AM, Fabien COELHO 
> wrote:
> >>
> >>
> >> Hello Masahiko,
> >>
>  So I would suggest to:
>   - fix the compilation issue
>   - leave -l/--log as it is, i.e. use "pgbench_log" as a prefix
>   - add --log-prefix=... (long option only) for changing this prefix
> >>>
> >>>
> >>> I agree. It's better to add the separated option to specify the prefix
> >>> of log file instead of changing the existing behaviour. Attached
> >>> latest patch incorporated review comments.
> >>> Please review it.
> >>
> >>
> >> Patch applies, compiles and works for me.
> >
> >
> > It works for me as well.
> >
> >>
> >>
> >> This new option is for benchmarking, so "benchmarking_option_set" should
> >> be set to true.
> >>
> >> To simplify the code, I would suggest to initialize explicitely
> >> "logfile_prefix" to the default value which is then overriden when the
> >> option is set, which allows to get rid of the "prefix" variable later.
> >>
> >> There is no reason to change the documentation by breaking a line at
> >> another place if the text is not changed (where ... with 1).
> >>
> >> I would suggest to simplify a little bit the documentation:
> >>   "prefix of log file ..." ->
> >>   "default log file prefix that can be changed with ..."
> >>
> >> Otherwise the explanations seem clear enough to me. If a native English
> >> speaker could check them though, it would be nice.
> >
> >
> > For the explanation of the option --log-prefix, I feel it would be
> better to
> > say "Custom prefix for transaction log file. Default is pgbench_log"
> >
> >
> > -   pgbench_log.nnn, where
> > +
> > pgbench_log.<
> replaceable>nnn,
> > +   where pgbench_log is the prefix of log
> file
> > that can
> > +   be changed by specifying --log-prefix, and where
> >
> > It could just say "the default prefix pgbench_log can be changed with
> option
> > --log-prefix, and " we need not use where again.
> >
> > Also the error message is made similar to that of aggregate-interval but
> I
> > think the one in sampling-rate is better:
> >
> > $ pgbench --log-prefix=chk -t 20
> > log file prefix (--log-prefix) is allowed only when actually logging
> > transactions
> >
> > pgbench  --sampling-rate=1 -t 20
> > log sampling (--sampling-rate) is allowed only when logging transactions
> > (-l)
> >
>
> Thank you for reviewing this patch!
>
> Attached latest patch incorporated comments.
> Please review it.
>
>
Thank you for updating the patch.

I note that the current changes, break the behaviour when we do not use -l
option.

Since the log_prefix variable is set to default value, the check " if
(!use_log && logfile_prefix)"  always returns true. This throws error even
when we have not used the -l and --log-prefix option as shown below.

$ pgbench -T 50
log file prefix (--log-prefix) is allowed only when logging transactions
(-l)

Kindly fix this.

Thank you,


--

Beena Emerson

Have a Great Day!

Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled

[MauMau]

From: Michael Paquier
Hm.. I have just tested HEAD, my patch and your patch using my patch
test on pg_ctl.c, but I am always getting pgwin32_is_service set to 0
when running pg_ctl start from a terminal, and set it to 1 when
running pg_ctl service to register the service startup. Could you
precise in which ways you started the Postgres instance and could you
post the patch of pg_ctl you used? I am afraid that I am taking it
incorrectly because I am not able to see any differences.

Also, did you test the patch I posted and were you able to see the
same differences as with your patch? I still think that my short patch
is logically correct but if the tests are not we are in a no-go
position for any fix posted on this thread.


Yes, I tested both your patch and mine.  I used the attached pg_ctl.c.
It adds -z option which disables SECURITY_SERVICE_RID.

I registered the service with "pg_ctl register -N pg -D
datadir -w -z -S demand -U myuser -P mypass", then started the service
with "net start pg".  The following messages were output in the server
log:

LOG:  pgwin32_is_admin = 0
LOG:  pgwin32_is_service = 0
LOG:  database system was shut down at 2016-11-07 22:04:46 JST
LOG:  MultiXact member wraparound protections are now enabled
LOG:  database system is ready to accept connections
LOG:  autovacuum launcher started

Without -z, the message becomes "pgwin32_is_service = 1".  And without
the win32security.c patch, "pgwin32_is_service = 1" is output.

I guess you registered the service without specifying the service
account with -U.  Then the service runs as the Local System account,
whence pgwin32_is_service() returns 1.

Regards
Takayuki Tsunakawa

/*
-
 *
 * pg_ctl --- start/stops/restarts the PostgreSQL server
 *
 * Portions Copyright (c) 1996-2016, PostgreSQL Global Development
Group
 *
 * src/bin/pg_ctl/pg_ctl.c
 *
 *
-
 */

#ifdef WIN32
/*
 * Need this to get defines for restricted tokens and jobs. And it
 * has to be set before any header from the Win32 API is loaded.
 */
#define _WIN32_WINNT 0x0501
#endif

#include "postgres_fe.h"

#include "libpq-fe.h"
#include "pqexpbuffer.h"

#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 

#ifdef HAVE_SYS_RESOURCE_H
#include 
#include 
#endif

#include "getopt_long.h"
#include "miscadmin.h"

/* PID can be negative for standalone backend */
typedef long pgpid_t;


typedef enum
{
SMART_MODE,
FAST_MODE,
IMMEDIATE_MODE
} ShutdownMode;


typedef enum
{
NO_COMMAND = 0,
INIT_COMMAND,
START_COMMAND,
STOP_COMMAND,
RESTART_COMMAND,
RELOAD_COMMAND,
STATUS_COMMAND,
PROMOTE_COMMAND,
KILL_COMMAND,
REGISTER_COMMAND,
UNREGISTER_COMMAND,
RUN_AS_SERVICE_COMMAND
} CtlCommand;

#define DEFAULT_WAIT60

static bool do_wait = false;
static bool del_service_rid = false;
static bool wait_set = false;
static int  wait_seconds = DEFAULT_WAIT;
static bool wait_seconds_arg = false;
static bool silent_mode = false;
static ShutdownMode shutdown_mode = FAST_MODE;
static int  sig = SIGINT;   /* default */
static CtlCommand ctl_command = NO_COMMAND;
static char *pg_data = NULL;
static char *pg_config = NULL;
static char *pgdata_opt = NULL;
static char *post_opts = NULL;
static const char *progname;
static char *log_file = NULL;
static char *exec_path = NULL;
static char *event_source = NULL;
static char *register_servicename = "PostgreSQL";   /* FIXME: +
version ID? */
static char *register_username = NULL;
static char *register_password = NULL;
static char *argv0 = NULL;
static bool allow_core_files = false;
static time_t start_time;

static char postopts_file[MAXPGPATH];
static char version_file[MAXPGPATH];
static char pid_file[MAXPGPATH];
static char backup_file[MAXPGPATH];
static char recovery_file[MAXPGPATH];
static char promote_file[MAXPGPATH];

#ifdef WIN32
static DWORD pgctl_start_type = SERVICE_AUTO_START;
static SERVICE_STATUS status;
static SERVICE_STATUS_HANDLE hStatus = (SERVICE_STATUS_HANDLE) 0;
static HANDLE shutdownHandles[2];
static pid_t postmasterPID = -1;

#define shutdownEvent shutdownHandles[0]
#define postmasterProcess shutdownHandles[1]
#endif


static void write_stderr(const char *fmt,...) pg_attribute_printf(1,
2);
static void do_advice(void);
static void do_help(void);
static void set_mode(char *modeopt);
static void set_sig(char *signame);
static void do_init(void);
static void do_start(void);
static void do_stop(void);
static void do_restart(void);
static void do_reload(void);
static void do_status(void);
static void do_promote(void);
static void do_kill(pgpid_t pid);
static void print_msg(const char *msg);
static void adjust_data_dir(void);

#ifdef WIN32
#if (_MSC_VER >= 1800)
#include 
#else
static bool IsWindowsXPOrGreater(void);
static bool 

Re: [HACKERS] Push down more full joins in postgres_fdw

[Etsuro Fujita]

On 2016/11/07 11:24, Etsuro Fujita wrote:

On 2016/11/04 19:55, Etsuro Fujita wrote:

Attached is an updated version of the patch.



I noticed that I have included an unrelated regression test in the
patch.  Attached is a patch with the test removed.


I noticed that I inadvertently removed some changes from that patch, so 
I fixed that.  Please find attached an updated version of the patch. 
I'm also attaching an updated version of another patch for evaluating 
PHVs remotely, which has been created on top of that patch.  Changes to 
the latter: I revised comments and added a bit more regression tests.


Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***
*** 109,114  typedef struct deparse_expr_cxt
--- 109,116 
  /* Handy macro to add relation name qualification */
  #define ADD_REL_QUALIFIER(buf, varno)	\
  		appendStringInfo((buf), "%s%d.", REL_ALIAS_PREFIX, (varno))
+ #define SS_TAB_ALIAS_PREFIX	"s"
+ #define SS_COL_ALIAS_PREFIX	"c"
  
  /*
   * Functions to determine whether an expression can be evaluated safely on
***
*** 167,172  static void appendConditions(List *exprs, deparse_expr_cxt *context);
--- 169,180 
  static void deparseFromExprForRel(StringInfo buf, PlannerInfo *root,
  	RelOptInfo *joinrel, bool use_alias, List **params_list);
  static void deparseFromExpr(List *quals, deparse_expr_cxt *context);
+ static void deparseRangeTblRef(StringInfo buf, PlannerInfo *root, RelOptInfo *foreignrel,
+    bool make_subquery, List **params_list);
+ static void appendSubselectAlias(StringInfo buf, int tabno, int ncols);
+ static void getSubselectAliasInfo(Var *node, RelOptInfo *foreignrel,
+ 	  int *tabno, int *colno);
+ static bool isSubqueryExpr(Var *node, RelOptInfo *foreignrel, int *tabno, int *colno);
  static void deparseAggref(Aggref *node, deparse_expr_cxt *context);
  static void appendGroupByClause(List *tlist, deparse_expr_cxt *context);
  static void appendAggOrderBy(List *orderList, List *targetList,
***
*** 990,999  deparseSelectSql(List *tlist, List **retrieved_attrs, deparse_expr_cxt *context)
  	 */
  	appendStringInfoString(buf, "SELECT ");
  
  	if (foreignrel->reloptkind == RELOPT_JOINREL ||
! 		foreignrel->reloptkind == RELOPT_UPPER_REL)
  	{
! 		/* For a join relation use the input tlist */
  		deparseExplicitTargetList(tlist, retrieved_attrs, context);
  	}
  	else
--- 998,1013 
  	 */
  	appendStringInfoString(buf, "SELECT ");
  
+ 	/*
+ 	 * Note: non-NIL tlist might be supplied for a baserel; if the baserel is
+ 	 * deparsed as a subquery, non-NIL tlist will be supplied for the baserel.
+ 	 * (See deparseRangeTblRef.)  In that case use deparseExplicitTargetList.
+ 	 */
  	if (foreignrel->reloptkind == RELOPT_JOINREL ||
! 		foreignrel->reloptkind == RELOPT_UPPER_REL ||
! 		tlist != NIL)
  	{
! 		/* Use the input tlist */
  		deparseExplicitTargetList(tlist, retrieved_attrs, context);
  	}
  	else
***
*** 1155,1165  deparseLockingClause(deparse_expr_cxt *context)
--- 1169,1188 
  	StringInfo	buf = context->buf;
  	PlannerInfo *root = context->root;
  	RelOptInfo *rel = context->scanrel;
+ 	PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) rel->fdw_private;
  	int			relid = -1;
  
  	while ((relid = bms_next_member(rel->relids, relid)) >= 0)
  	{
  		/*
+ 		 * Ignore relation if it appears in a lower subquery, because in that
+ 		 * case we would have already considered locking for the relation
+ 		 * while deparsing the lower subquery.
+ 		 */
+ 		if (bms_is_member(relid, fpinfo->subquery_rels))
+ 			continue;
+ 
+ 		/*
  		 * Add FOR UPDATE/SHARE if appropriate.  We apply locking during the
  		 * initial row fetch, rather than later on as is done for local
  		 * tables. The extra roundtrips involved in trying to duplicate the
***
*** 1347,1364  deparseFromExprForRel(StringInfo buf, PlannerInfo *root, RelOptInfo *foreignrel,
  
  	if (foreignrel->reloptkind == RELOPT_JOINREL)
  	{
- 		RelOptInfo *rel_o = fpinfo->outerrel;
- 		RelOptInfo *rel_i = fpinfo->innerrel;
  		StringInfoData join_sql_o;
  		StringInfoData join_sql_i;
  
  		/* Deparse outer relation */
  		initStringInfo(_sql_o);
! 		deparseFromExprForRel(_sql_o, root, rel_o, true, params_list);
  
  		/* Deparse inner relation */
  		initStringInfo(_sql_i);
! 		deparseFromExprForRel(_sql_i, root, rel_i, true, params_list);
  
  		/*
  		 * For a join relation FROM clause entry is deparsed as
--- 1370,1391 
  
  	if (foreignrel->reloptkind == RELOPT_JOINREL)
  	{
  		StringInfoData join_sql_o;
  		StringInfoData join_sql_i;
  
  		/* Deparse outer relation */
  		initStringInfo(_sql_o);
! 		deparseRangeTblRef(_sql_o, root,
! 		   fpinfo->outerrel,
! 		   fpinfo->make_outerrel_subquery,
! 		   params_list);
  
  		/* Deparse inner relation */
  		initStringInfo(_sql_i);
! 		deparseRangeTblRef(_sql_i, root,
! 		   fpinfo->innerrel,
! 		 

Re: [HACKERS] Gather Merge

[Amit Kapila]

On Fri, Nov 4, 2016 at 8:30 AM, Thomas Munro
 wrote:
> On Thu, Oct 27, 2016 at 10:50 PM, Rushabh Lathia
>  wrote:
>> Please find attached latest patch which fix the review point as well as
>> additional clean-up.
>
> +/*
> + * Read the tuple for given reader into nowait mode, and form the tuple 
> array.
> + */
> +static void
> +form_tuple_array(GatherMergeState *gm_state, int reader)
>
> This function is stangely named.  How about try_to_fill_tuple_buffer
> or something?
>

Hmm.  We have discussed upthread to name it as form_tuple_array.  Now,
you feel that is also not good, I think it is basically matter of
perspective, so why not leave it as it is for now and we will come
back to naming it towards end of patch review or may be we can leave
it for committer to decide.

-- 
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] macaddr 64 bit (EUI-64) datatype support

[Shay Rojansky]

>
> > As I said before, Npgsql for one loads data types by name, not by OID.
>>> > So this would definitely cause breakage.
>>>
>>> Why would that cause breakage?
>>
>>
>> Well, the first thing Npgsql does when it connects to a new database, is
>> to query pg_type. The type names are used to associate entries with type
>> handlers, avoiding the hard-coding of OIDs in code. So if the type name
>> "macaddr" suddenly has a new meaning and its wire representation is
>> different breakage will occur. It is possible to release new versions of
>> Npgsql which will look at the PostgreSQL version and say "we know that in
>> PostgreSQL < 10 macaddr means this, but in >= 10 it means that". But that
>> doesn't seem like a good solution, plus old versions of Npgsql from before
>> this change won't work.
>>
>
> The new datatype that is going to replace the existing one works with both
> 6 and 8 byte
> MAC address and stores it a variable length format. This doesn't change
> the wire format.
> I don't see any problem with the existing applications. The new datatype
> can recv and send
> 8 byte MAC address also.
>

Apologies, I may have misunderstood. If the new type is 100%
wire-compatible (recv/send) with the old fixed-length 6-byte type, then
there's no issue whatsoever.

Re: [HACKERS] Radix tree for character conversion

[Daniel Gustafsson]

> On 04 Nov 2016, at 08:34, Kyotaro HORIGUCHI  
> wrote:
> 
> Thank you for looling this.

And thank you for taking the time to read my patches!

> At Mon, 31 Oct 2016 17:11:17 +0100, Daniel Gustafsson  wrote 
> in <3fc648b5-2b7f-4585-9615-207a44b73...@yesql.se>
>>> On 27 Oct 2016, at 09:23, Kyotaro HORIGUCHI 
>>>  wrote:
>>> Perl scripts are to be messy, I believe. Anyway the duplicate
>>> check as been built into the sub print_radix_trees. Maybe the
>>> same check is needed by some plain map files but it would be just
>>> duplication for the maps having radix tree.
>> 
>> I took a small stab at doing some cleaning of the Perl scripts, mainly around
>> using the more modern (well, modern as in +15 years old) form for open(..),
>> avoiding global filehandles for passing scalar references and enforcing use
>> strict.  Some smaller typos and fixes were also included.  It seems my Perl 
>> has
>> become a bit rusty so I hope the changes make sense.  The produced files are
>> identical with these patches applied, they are merely doing cleaning as 
>> opposed
>> to bugfixing.
>> 
>> The attached patches are against the 0001-0006 patches from Heikki and you in
>> this series of emails, the separation is intended to make them easier to 
>> read.
> 
> I'm not sure how the discussion about this goes, these patches
> makes me think about coding style of Perl.

Some of this can absolutely be considered style and more or less down to
personal preference.  I haven’t seen any coding conventions for Perl so I
assume it’s down to consensus among the committers.  My rationale for these
patches in the first place was that I perceived this thread to partly want to
clean up the code and make it more modern Perl.

> The distinction between executable script and library is by
> intention with an obscure basis. Existing scripts don't get less
> modification, but library uses more restricted scopes to get rid
> of the troubles caused by using global scopes. But I don't have a
> clear preference on that. The TAP test scripts takes OO notations
> but I'm not sure convutils.pl also be better to take the same
> notation. It would be rarely edited hereafter and won't gets
> grown any more.

I think the current convutils module is fine and converting it to OO would be
overkill.

> As far as I see the obvious bug fixes in the patchset are the
> following,

Agreed, with some comments:

> - 0007: load_maptable fogets to close input file.

An interesting note on this is that it’s not even a bug =) Since $in is a
scalar reference, there is no need to explicitly close() the filehandle since
the reference counter will close it on leaving scope, but there’s no harm in
doing it ourselves and it also makes for less confusion for anyone not familiar
with Perl internals.

> - 0010: commment for load_maptables is wrong.

There is also a fix for a typo in make_mapchecker.pl

> - 0011: hash reference is incorrectly dereferenced
> 
> All other fixes other than the above three seem to be styling or
> syntax-generation issues and I don't know whether any
> recommendation exists…

I think there are some more fixes that arent styling/syntax remaining.  I’ll go
through the patches one by one:

0007 - While this might be considered styling/syntax, my $0.02 is that it’s not
and instead a worthwhile change.  I’ll illustrate with an example from the
patch in question:

Using a bareword global variable in open() for the filehandle was replaced with
the three-part form in 5.6 and is now even actively discouraged from in the
Perl documentation (and has been so since the 5.20 docs).  The problem is that
they are global and can thus easily clash, so easily that the 0007 patch
actually fixes one such occurrence:

print_radix_map() opens the file in the global filehandle OUT and passes it to
print_radix_table() with the typeglob *OUT; print_radit_table() in turn passes
the filehandle to print_segmented_table() which writes to the file using the
parameter $hd, except in one case where it uses the global OUT variable without
knowing it will be the right file.  This is where the hunk below in 0007 comes
in:

-   print OUT "$line\n";
+   print { $$hd } "$line\n";

In this case OUT references the right file and it produces the right result,
but it illustrates how easy it is to get wrong (which can cause very subtle
bugs).  So, when poking at this code we might as well, IMHO, use what is today
in Perl considered the right way to deal with filehandle references.

Using implicit filemodes can also introduce bugs when opening filenames passed
in from the outside as we do in UCS_to_most.pl.  Considering the use case of
these scripts it’s obviously quite low on the list of risks but still.

0008 - I don’t think there are any recommendations whether or not to use use
strict; in the codebase, there certainly are lots of scripts not doing it.
Personally I think it’s good 

Re: [HACKERS] Specifying the log file name of pgbench -l option

[Masahiko Sawada]

On Wed, Nov 2, 2016 at 3:57 PM, Beena Emerson  wrote:
> Hello,
>
> On Wed, Nov 2, 2016 at 4:16 AM, Fabien COELHO  wrote:
>>
>>
>> Hello Masahiko,
>>
 So I would suggest to:
  - fix the compilation issue
  - leave -l/--log as it is, i.e. use "pgbench_log" as a prefix
  - add --log-prefix=... (long option only) for changing this prefix
>>>
>>>
>>> I agree. It's better to add the separated option to specify the prefix
>>> of log file instead of changing the existing behaviour. Attached
>>> latest patch incorporated review comments.
>>> Please review it.
>>
>>
>> Patch applies, compiles and works for me.
>
>
> It works for me as well.
>
>>
>>
>> This new option is for benchmarking, so "benchmarking_option_set" should
>> be set to true.
>>
>> To simplify the code, I would suggest to initialize explicitely
>> "logfile_prefix" to the default value which is then overriden when the
>> option is set, which allows to get rid of the "prefix" variable later.
>>
>> There is no reason to change the documentation by breaking a line at
>> another place if the text is not changed (where ... with 1).
>>
>> I would suggest to simplify a little bit the documentation:
>>   "prefix of log file ..." ->
>>   "default log file prefix that can be changed with ..."
>>
>> Otherwise the explanations seem clear enough to me. If a native English
>> speaker could check them though, it would be nice.
>
>
> For the explanation of the option --log-prefix, I feel it would be better to
> say "Custom prefix for transaction log file. Default is pgbench_log"
>
>
> -   pgbench_log.nnn, where
> +
> pgbench_log.nnn,
> +   where pgbench_log is the prefix of log file
> that can
> +   be changed by specifying --log-prefix, and where
>
> It could just say "the default prefix pgbench_log can be changed with option
> --log-prefix, and " we need not use where again.
>
> Also the error message is made similar to that of aggregate-interval but I
> think the one in sampling-rate is better:
>
> $ pgbench --log-prefix=chk -t 20
> log file prefix (--log-prefix) is allowed only when actually logging
> transactions
>
> pgbench  --sampling-rate=1 -t 20
> log sampling (--sampling-rate) is allowed only when logging transactions
> (-l)
>

Thank you for reviewing this patch!

Attached latest patch incorporated comments.
Please review it.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 285608d..a6fe183 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -614,6 +614,15 @@ pgbench  options  dbname
   
  
 
+ 
+  --log-prefix=prefix
+  
+   
+Custom prefix of transaction log file. Default is pgbench_log.
+   
+  
+ 
+
 

 
@@ -1121,13 +1130,14 @@ END;
With the -l option but without the --aggregate-interval,
pgbench writes the time taken by each transaction
to a log file.  The log file will be named
-   pgbench_log.nnn, where
-   nnn is the PID of the pgbench process.
-   If the -j option is 2 or higher, creating multiple worker
-   threads, each will have its own log file. The first worker will use the
-   same name for its log file as in the standard single worker case.
+   pgbench_log.nnn,
+   where the default prefix pgbench_log can be changed with option
+   --log-prefix, and nnn is the PID of the
+   pgbench process. If the -j option is 2 or higher,
+   creating multiple worker threads, each will have its own log file. The first worker will
+   use the same name for its log file as in the standard single worker case.
The additional log files for the other workers will be named
-   pgbench_log.nnn.mmm,
+   pgbench_log.nnn.mmm,
where mmm is a sequential number for each worker starting
with 1.
   
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index d44cfda..c7c1ac6 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -180,6 +180,7 @@ char	   *pghost = "";
 char	   *pgport = "";
 char	   *login = NULL;
 char	   *dbName;
+char	   *logfile_prefix = "pgbench_log";
 const char *progname;
 
 #define WSEP '@'/* weight separator */
@@ -511,6 +512,7 @@ usage(void)
 		   "  --aggregate-interval=NUM aggregate data over NUM seconds\n"
 		"  --progress-timestamp use Unix epoch timestamps for progress\n"
 		   "  --sampling-rate=NUM  fraction of transactions to log (e.g., 0.01 for 1%%)\n"
+		   "  --log-prefix=PREFIX  prefix of transaction time log file (default: pgbench_log)\n"
 		   "\nCommon options:\n"
 		   "  -d, --debug  print debugging output\n"
 	  "  -h, --host=HOSTNAME  database server host or socket directory\n"
@@ -3643,6 +3645,7 @@ main(int argc, char **argv)
 		{"sampling-rate", required_argument, NULL, 4},
 		{"aggregate-interval", required_argument, NULL, 5},
 		{"progress-timestamp", 

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Albe Laurenz]

Tom Lane wrote:
>> Albe Laurenz  writes:
>>> Anyway, I have prepared a patch along the lines you suggest.
>> 
>> Pushed, we'll see if the buildfarm likes this iteration any better.
> 
> And the answer is "not very much".  The Windows builds aren't actually
> failing, but they are producing lots of warnings:
> 
> lquery_op.obj : warning LNK4197: export '_ltq_regex' specified multiple 
> times; using first specification
[...]
> 
> This is evidently from the places where there are two "extern"
> declarations for a function, one in a header and one in
> PG_FUNCTION_INFO_V1.  The externs are identical now, but nonetheless
> MSVC insists on whining about it.

Funny -- it seems to mind a duplicate declaration only if there
is PGDLLEXPORT in at least one of them.

> I'm inclined to give this up as a bad job and go back to the
> previous state.  We have a solution that works and doesn't
> produce warnings; third-party authors who don't want to use it
> are on their own.

I think you are right.

Thanks for the work and thought you expended on this!
Particularly since Windows is not your primary interest.

Yours,
Laurenz Albe

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

Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled

[Michael Paquier]

On Mon, Nov 7, 2016 at 9:49 AM, Tsunakawa, Takayuki
 wrote:
> From: pgsql-hackers-ow...@postgresql.org
>> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
>> On Sun, Nov 6, 2016 at 6:30 PM, MauMau  wrote:
>> So you see the same behavior with the patch I sent and your refactoring,
>> right? If yes, backpatching the one-liner is the safest bet to me. We could
>> keep the refactoring for HEAD if it makes sense.
>
> Yes.  And It's fine to me that your patch will be applied to previous 
> releases and my patch to HEAD only.  This is a good (rare?) chance to reduce 
> the Windows-specific code, so I want to take advantage of it.

Yes, I can follow that argument.

>> Something is wrong with the format of your patch by the way. My Windows
>> and even OSX environments recognize it as a binary file, though I can read
>> it in any editor and I cannot apply it cleanly with a simple patch command.
>> Could you send it again and double-check?
>
> Ouch, the Git shell included in GitHub Desktop for Windows produced the diff 
> in UTF-16 and CR/LF line terminators.  I haven't found how to fix it, so I 
> generated the attached patch on Linux.  Please check it.

And the patch got twice smaller in size. Thanks.

>> > To reproduce the OP's problem, I modified pg_ctl.c to disable
>> > SECURITY_SERVICE_RID when spawning postgres.exe.
>>
>> So basically you allocated a SID to drop via AllocateAndInitializeSid,
>> called _CreateRestrictedToken and let the process being spawned? I think
>> that this is the patch attached (win32-disable-service-rid.patch). Could
>> you confirm? I want to be sure that we are testing the same things.
>
> Yes, I did the same.

Hm.. I have just tested HEAD, my patch and your patch using my patch
test on pg_ctl.c, but I am always getting pgwin32_is_service set to 0
when running pg_ctl start from a terminal, and set it to 1 when
running pg_ctl service to register the service startup. Could you
precise in which ways you started the Postgres instance and could you
post the patch of pg_ctl you used? I am afraid that I am taking it
incorrectly because I am not able to see any differences.

Also, did you test the patch I posted and were you able to see the
same differences as with your patch? I still think that my short patch
is logically correct but if the tests are not we are in a no-go
position for any fix posted on this thread.
-- 
Michael


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