Re: [HACKERS] Planning counters in pg_stat_statements

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Thomas Munro
> I have often wanted $SUBJECT and was happy to find that Fujii-san had posted
> a patch five years ago[1].  The reception then seemed positive.
> So here is a refurbished and (hopefully) improved version of his patch with
> a new column for the replan count.  Thoughts?

That's a timely proposal.  I sometimes faced performance problems where the 
time pg_stat_statements shows is much shorter than the application perceives.  
The latest experience was that the execution time of a transaction, which 
consists of dozens of DMLs and COMMIT, was about 200ms from the application's 
perspective, while pg_stat_statements showed only about 10ms in total.  The 
network should not be the cause because the application ran on the same host as 
the database server.  I wanted to know how long the parsing and planning time 
was.

BTW, the current pg_stat_statement shows unexpected time for COMMIT.  I expect 
it to include the whole COMMIT processing, including the long WAL flush and 
sync rep wait.  However, it only shows the time for the transaction state 
change in memory.

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] Statement-level rollback

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Simon Riggs
> A backend-based solution is required for PL procedures and functions.
> 
> We could put this as an option into PL/pgSQL, but it seems like it is
> a function of the transaction manager rather than the driver.

Exactly.  Thanks.

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] Statement-level rollback

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Vladimir
> Sitnikov
> Tsunakawa> PgJDBC has supported the feature with autosave parameter only
> Tsunakawa> recently
> 
> PgJDBC has the implementation for more than a year (REL9.4.1210, 2016-09-07,
> see https://github.com/pgjdbc/pgjdbc/pull/477 )

And I heard from someone in PgJDBC community that the autosave parameter was 
not documented in the manual for a while, which I confirmed.  So the 
statement-level rollback is newer to users, isn't it?


> The performance overhead for "SELECT" statement (no columns, just select)
> statement over localhost is 36±4 us vs 38±3 us (savepoint is pipelined along
> with user-provided query). That is network overhead is close to negligible.

That's good news, because it also means that the overhead of creating a 
savepoint is negligible.



> As far as I understand, the main problem with savepoints is they would
> consume memory even in case the same savepoint is reassigned again and again.
> In other words, "savepoint; insert;savepoint; insert;savepoint;
> insert;savepoint; insert;savepoint; insert;" would allocate xids and might
> blow up backend's memory.
> I see no way driver can workaround that, so it would be great if backend
> could release memory or provide a way to do so.

Doesn't PgJDBC execute RELEASE after each SQL statement?  That said, even with 
RELEASE, the server memory bloat is not solved.  The current server 
implementation allocates a memory chunk of 8KB called CurTranContext for each 
subtransaction, and retains them until the end of top-level transaction.  
That's another (separate) issue to address.

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] Statement-level rollback

[Tsunakawa, Takayuki]

From: Craig Ringer [mailto:cr...@2ndquadrant.com]
> The example often cited is some variant of
> 
> BEGIN;
> CREATTE TABLE t2 AS SELECT * FROM t1;
> DROP TABLE t1;
> ALTER TABLE t2 RENAME TO t1;
> COMMIT;
> 
> Right now, we do the right thing here. With default statement level rollback,
> you just dropped t1 and all your data. oops.

That's a horrible example.  So I think the default behavior should be what it 
is now for existing PostgreSQL users.


> On a related note, psql's -v ON_ERROR_STOP=1 is horrible and hard to discover
> UI, and one of the top FAQs on Stack Overflow is some variant of "I'm getting
> random and incomprehensible errors restoring a dump, wtf?". So I'd really
> love to make it the default, but we'd face similar issues where a SQL script
> that's currently correct instead produces dangerously wrong results with
> ON_ERROR_STOP=1 .

Yes.  And although unrelated, psql's FETCH_SIZE is also often invisible to 
users.  They report out-of-memory trouble when they do SELECT on a large table 
with psql.



> What about if we add protocol-level savepoint support? Two new messages:
> 
> BeginUnnamedSavepoint
> 
> and
> 
> EndUnnamedSavepoint
> 
> where the latter does a rollback-to-last-unnamed-savepoint if the txn state
> is bad, or a release-last-unnamed-savepoint if the txn state is ok. That
> means the driver doesn't have to wait for the result of the statement. It
> knows the conn state and query outcome from our prior messages, and knows
> that as a result of this message any failed state has been rolled back.
> 
> This would, with appropriate libpq support, give people who want statement
> level error handling pretty much what they want. And we could expose it
> in psql too. No GUCs needed, no fun surprises for apps. psqlODBC could adopt
> it to replace its current slow and super-log-spammy statement rollback
> model.
> 
> Downside is that it needs support in each client driver.

Yes, I believe we should avoid the downside.  It's tough to develop and 
maintain a client driver, so we should minimize the burdon with server-side 
support.

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] Statement-level rollback

[Tsunakawa, Takayuki]

From: Peter Eisentraut [mailto:peter.eisentr...@2ndquadrant.com]
> The difference is how error recovery works.  So this will necessarily be
> tied to how the client code or other surrounding code is structured or what
> the driver or framework is doing in the background to manage transactions.
> It would also be bad if client code was not prepared for this new behavior,
> reported the transaction as complete while some commands in the middle were
> omitted.
> 
> Drivers can already achieve this behavior and do do that by issuing savepoint
> commands internally.  The point raised in this thread was that that creates
> too much network overhead, so a backend-based solution would be preferable.
> We haven't seen any numbers or other evidence to quantify that claim, so
> maybe it's worth looking into that some more.
> 
> In principle, a backend-based solution that drivers just have to opt into
> would save a lot of duplication.  But the drivers that care or require it
> according to their standards presumably already implement this behavior
> in some other way, so it comes back to whether there is a performance or
> other efficiency gain here.
> 
> Another argument was that other SQL implementations have this behavior.
> This appears to be the case.  But as far as I can tell, it is also tied
> to their particular interfaces and the structure and flow control they
> provide.  So a client-side solution like psql already provides or something
> in the various drivers would work just fine here.
> 
> So my summary for the moment is that a GUC or similar run-time setting might
> be fine, with appropriate explanation and warnings.  But it's not clear
> whether it's worth it given the existing alternatives.

I can think of four reasons why the server-side support is necessary or 
desirable.

First, the server log could be filled with SAVEPOINT and RELEASE lines when you 
need to investigate performance or audit activity.

Second, the ease of use for those who migrate from other DBMSs.  With the 
server-side support, only the DBA needs to be aware of the configuration in 
postgresql.conf.  Other people don't need to be aware of the client-side 
parameter when they deploy applications.

Third, lack of server-side support causes trouble to driver developers.  In a 
recent discussion with the psqlODBC committer, he had some trouble improving or 
fixing the statement-rollback support.  Npgsql doesn't have the 
statement-rollback yet.  PgJDBC has supported the feature with autosave 
parameter only recently.  Do the drivers for other languages like Python, Go, 
JavaScript have the feature?  We should reduce the burdon on the driver 
developers.

Fourth, the runtime performance.  In a performance benchmark of one of our 
customers, where a batch application ran 1.5 or 5 million small SELECTs with 
primary key access, the execution time of the whole batch became shorter by 
more than 30% (IIRC) when the local connection was used instead of the remote 
TCP/IP one.  The communication overhead is not small.

Also, in the PostgreSQL documentation, the communication overhead is treated 
seriously as follows:


https://www.postgresql.org/docs/devel/static/plpgsql-overview.html#plpgsql-advantages

[Excerpt]
--
That means that your client application must send each query to the database 
server, wait for it to be processed, receive and process the results, do some 
computation, then send further queries to the server. All this incurs 
interprocess communication and will also incur network overhead if your client 
is on a different machine than the database server.

With PL/pgSQL you can group a block of computation and a series of queries 
inside the database server, thus having the power of a procedural language and 
the ease of use of SQL, but with considerable savings of client/server 
communication overhead.


•Extra round trips between client and server are eliminated


•Intermediate results that the client does not need do not have to be marshaled 
or transferred between server and client


•Multiple rounds of query parsing can be avoided


This can result in a considerable performance increase as compared to an 
application that does not use stored functions.
--


Craig reports the big communication overhead:

PATCH: Batch/pipelining support for libpq
https://www.postgresql.org/message-id/CAMsr+YFUjJytRyV4J-16bEoiZyH=4nj+sQ7JP9ajwz=b4dm...@mail.gmail.com#CAMsr+YFUjJytRyV4J-16bEoiZyH=4nj+sQ7JP9ajwz=b4dm...@mail.gmail.com

Re: foreign table batch insert
https://www.postgresql.org/message-id/CAMsr+YFgDUiJ37DEfPRk8WDBuZ58psdAYJd8iNFSaGxtw=w...@mail.gmail.com

[Excerpt]
--
The time difference for 10k inserts on the local host over a unix socket
shows a solid improvement:

batch insert elapsed:  0.244293s
sequential insert elapsed: 0.375402s

... but over, say, a connection to a random 

Re: [HACKERS] [bug fix] postgres.exe crashes with access violation on Windows while starting up

[Tsunakawa, Takayuki]

From: Michael Paquier [mailto:michael.paqu...@gmail.com]
> On Tue, Oct 31, 2017 at 6:59 AM, Tsunakawa, Takayuki
> <tsunakawa.ta...@jp.fujitsu.com> wrote:
> > When CurrentMemoryContext is NULL, the message  is logged with
> ReportEventA().  This is similar to write_console().
> 
> My point is that as Postgres is running as a service, isn't it wrong to
> write a message to stderr as a fallback if the memory context is not set?
> You would lose a message. It seems to me that for an operation that can
> happen at a low-level like the postmaster startup, we should really use
> a low-level operation as well.

I'm sorry I may not have been clear.  With this patch, write_eventlog() outputs 
the message to event log with ReportEventA() when CurrentMemoryContext is NULL. 
 It doesn't write to stderr.  So the message won't be lost.

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] [bug fix] postgres.exe crashes with access violation on Windows while starting up

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> So you are basically ready to lose any message that could be pushed
> here if there is no memory context? That does not sound like a good
> trade-off to me. A static buffer does not look like the best idea
> either to not truncate message, so couldn't we envisage to just use
> malloc? pgwin32_message_to_UTF16() is called in two places in elog.c,
> and there is a full control on the error code paths.

Thank you for reviewing a rare bug fix on Windows that most people wouldn't be 
interested in. When CurrentMemoryContext is NULL, the message  is logged with 
ReportEventA().  This is similar to write_console().

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] proposal: schema variables

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Pavel Stehule
> I propose a  new database object - a variable. The variable is persistent
> object, that holds unshared session based not transactional in memory value
> of any type. Like variables in any other languages. The persistence is
> required for possibility to do static checks, but can be limited to session
> - the variables can be temporal.
> 
> 
> My proposal is related to session variables from Sybase, MSSQL or MySQL
> (based on prefix usage @ or @@), or package variables from Oracle (access
> is controlled by scope), or schema variables from DB2. Any design is coming
> from different sources, traditions and has some advantages or disadvantages.
> The base of my proposal is usage schema variables as session variables for
> stored procedures. It should to help to people who try to port complex
> projects to PostgreSQL from other databases.

Very interesting.  I hope I could join the review and testing.

How do you think this would contribute to easing the port of Oracle PL/SQL 
procedures?  Would the combination of orafce and this feature promote 
auto-translation of PL/SQL procedures?  I'm curious what will be the major road 
blocks after adding the schema variable.

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

[HACKERS] Re: [bug fix] postgres.exe crashes with access violation on Windows while starting up

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tsunakawa,
> Takayuki
> The reason is for not outputing the crash dump is a) the crash occurred
> before installing the Windows exception handler
> (pgwin32_install_crashdump_handler() call) and b) the effect of the
> following call in postmaster is inherited in the child process.
> 
>   /* In case of general protection fault, don't show GUI popup
> box */
>   SetErrorMode(SEM_FAILCRITICALERRORS |
> SEM_NOGPFAULTERRORBOX);
> 
> But I'm not sure in what order we should do
> pgwin32_install_crashdump_handler(), startup_hacks() and steps therein,
> MemoryContextInit().  I think that's another patch.

Just installing the handler at the beginning of main() seems fine.  Patch 
attached.

Regards
Takayuki Tsunakawa
 


crash_dump_before_installing_handler.patch
Description: crash_dump_before_installing_handler.patch

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

[HACKERS] [bug fix] postgres.exe crashes with access violation on Windows while starting up

[Tsunakawa, Takayuki]

Hello,

We encountered a rare and hard-to-investigate problem on Windows, which one of 
our customers reported.  Please find the attached patch to fix that.  I'll add 
this to the next CF.


PROBLEM
==

PostgreSQL sometimes crashes with the following messages.  This is infrequent 
(but frequent for the customer); it occurred about 10 times in the past 5 
months.

LOG:  server process (PID 2712) was terminated by exception 0xC005
HINT:  See C include file "ntstatus.h" for a description of the hexadecimal 
value.
LOG:  terminating any other active server processes
WARNING:  terminating connection because of crash of another server process
DETAIL:  The postmaster has commanded this server process to roll back the 
current transaction and exit, because another server process exited abnormally 
and possibly corrupted shared memory.
HINT:  In a moment you should be able to reconnect to the database and repeat 
your command.
LOG:  all server processes terminated; reinitializing

"server process" shows that an client backend crashed.  The above messages 
indicate that the process was not running an SQL command.

PostgreSQL runs as a Windows service.

No crash dump was produced anywhere, despite the facts:
- /crashdumps folder exists and is writable by the PostgreSQL user 
account (which is the user postgres.exe runs as)
- The Windows registry configuration allows dumping the crash dump


CAUSE
==

We believe WSAStartup() in main.c failed.  The only conceivable error is:

WSAEPROCLIM
10067
Too many processes.
A Windows Sockets implementation may have a limit on the number of applications 
that can use it simultaneously. WSAStartup may fail with this error if the 
limit has been reached.

But I couldn't find what the limit is and whether we can tune it.  We couldn't 
reproduce the problem.

When I pretend that WSAStartup() failed while a client backend is starting up, 
I could see the same phenomenon as the customer.  This problem only occurs when 
PostgreSQL runs as a Windows service.

The bug is in write_eventlog().  It calls pgwin32_message_to_utf16() which in 
turn calls palloc(), which requires the memory management system to be set up 
(CurrentMemoryContext != NULL).


FIX
==

Add the check "CurrentMemoryContext != NULL" in write_eventlog() as in 
write_console().


NOTE
==

The reason is for not outputing the crash dump is a) the crash occurred before 
installing the Windows exception handler (pgwin32_install_crashdump_handler() 
call) and b) the effect of the following call in postmaster is inherited in the 
child process.

/* In case of general protection fault, don't show GUI popup 
box */
SetErrorMode(SEM_FAILCRITICALERRORS | SEM_NOGPFAULTERRORBOX);

But I'm not sure in what order we should do 
pgwin32_install_crashdump_handler(), startup_hacks() and steps therein, 
MemoryContextInit().  I think that's another patch.

Regards
Takayuki Tsunakawa




write_eventlog_crash.patch
Description: write_eventlog_crash.patch

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

Re: [HACKERS] Remove secondary checkpoint

[Tsunakawa, Takayuki]

From: Alvaro Herrera [mailto:alvhe...@alvh.no-ip.org]
> Tsunakawa, Takayuki wrote:
> 
> > (Although unrelated to this, I've also been wondering why PostgreSQL
> > flushes WAL to disk when writing a page in the shared buffer, because
> > PostgreSQL doesn't use WAL for undo.)
> 
> The reason is that if the system crashes after writing the data page to
> disk, but before writing the WAL, the data page would be inconsistent with
> data in pages that weren't flushed, since there is no WAL to update those
> other pages.  Also, if the system crashes after partially writing the page
> (say it writes the first 4kB) then the page is downright corrupted with
> no way to fix it.
> 
> So there has to be a barrier that ensures that the WAL is flushed up to
> the last position that modified a page (i.e. that page's LSN) before actually
> writing that page to disk.  And this is why we can't use mmap() for shared
> buffers -- there is no mechanism to force the WAL down if the operation
> system has the liberty to flush pages whenever it likes.

I see.  The latter is a torn page problem, which is solved by a full page image 
WAL record.  I understood that an example of the former problem is the 
inconsistency between a table page and an index page -- if an index page is 
flushed to disk without slushing the WAL and the corresponding table page, an 
index entry would point to a wroing table record after recovery.

Thanks, my long-standing question has beenn solved.


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] [bug fix] ECPG: fails to recognize embedded parameters

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Meskes
> Thanks for spotting and fixing. I just committed your patch to master and
> backported to 9.4, 9.5, 9.6 and 10. It doesn't apply cleanly to 9.3. But
> then it might not be important enough to investigate and backported to this
> old a version.

Thanks.  I'm OK with 9.3.

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] Remove secondary checkpoint

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Simon Riggs
> This
> * reduces disk space requirements on master
> * removes a minor bug in fast failover
> * simplifies code

I welcome this patch.  I was wondering why PostgreSQL retains the previous 
checkpoint.  (Although unrelated to this, I've also been wondering why 
PostgreSQL flushes WAL to disk when writing a page in the shared buffer, 
because PostgreSQL doesn't use WAL for undo.)

Here are my review comments.

(1)
-* a) we keep WAL for two checkpoint cycles, back to the "prev" 
checkpoint.
+* a) we keep WAL for only one checkpoint cycle (prior to PG11 we kept
+*WAL for two checkpoint cycles to allow us to recover from the
+*secondary checkpoint if the first checkpoint failed, though we
+*only did this on the master anyway, not on standby. Keeping just
+*one checkpoint simplifies processing and reduces disk space in
+*many smaller databases.)

/*
-* The last valid checkpoint record required for a 
streaming
-* recovery exists in neither standby nor the primary.
+* We used to attempt to go back to a secondary 
checkpoint
+* record here, but only when not in standby_mode. We 
now
+* just fail if we can't read the last checkpoint 
because
+* this allows us to simplify processing around 
checkpoints.
 */


if (fast_promote)
{
-   checkPointLoc = ControlFile->prevCheckPoint;
+   /*
+* It appears to be a bug that we used to use 
prevCheckpoint here
+*/
+   checkPointLoc = ControlFile->checkPoint;

-   XLByteToSeg(PriorRedoPtr, _logSegNo, wal_segment_size);
+   /* Trim from the last checkpoint, not the last - 1 */
+   XLByteToSeg(RedoRecPtr, _logSegNo, wal_segment_size);
 
I suggest to remove references to the secondary checkpoint (the old behavior) 
from the comments.  I'm afraid those could confuse new developers.



(2)
@@ -8110,10 +8100,6 @@ ReadCheckpointRecord(XLogReaderState *xlogreader, 
XLogRecPtr RecPtr,
ereport(LOG,
(errmsg("invalid primary 
checkpoint link in control file")));
break;

@@ -8135,10 +8121,6 @@ ReadCheckpointRecord(XLogReaderState *xlogreader, 
XLogRecPtr RecPtr,
ereport(LOG,
(errmsg("invalid primary 
checkpoint record")));
break;

@@ -8154,10 +8136,6 @@ ReadCheckpointRecord(XLogReaderState *xlogreader, 
XLogRecPtr RecPtr,
ereport(LOG,
(errmsg("invalid resource 
manager ID in primary checkpoint record")));
break;

etc, etc.

"primary checkpoint" should be just "checkpoint", now that the 
primary/secondary concept will disappear.


(3)
Should we change the default value of max_wal_size from 1 GB to a smaller size? 
 I vote for "no" for performance.

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] Remove secondary checkpoint

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> On Tue, Oct 24, 2017 at 5:58 PM, Tsunakawa, Takayuki
> <tsunakawa.ta...@jp.fujitsu.com> wrote:
> > If the latest checkpoint record is unreadable (the WAL
> segment/block/record is corrupt?), recovery from the previous checkpoint
> would also stop at the latest checkpoint.  And we don't need to replay the
> WAL records between the previous checkpoint and the latest one, because
> their changes are already persisted when the latest checkpoint was taken.
> So, the user should just do pg_resetxlog and start the database server when
> the recovery cannot find the latest checkpoint record and PANICs?
> 
> Not necessarily. If a failure is detected when reading the last checkpoint,
> as you say recovery would begin at the checkpoint prior that and would stop
> when reading the record of last checkpoint, still one could use a
> recovery.conf with restore_command to fetch segments from a different
> source, like a static archive, as only the local segment may be corrupted.

Oh, you are right.  "If the crash recovery fails, perform recovery from backup."

Anyway, I agree that the secondary checkpoint isn't necessary.

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] Remove secondary checkpoint

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tom Lane
> Doesn't it also make crash recovery less robust?  The whole point
> of that mechanism is to be able to cope if the latest checkpoint
> record is unreadable.

If the latest checkpoint record is unreadable (the WAL segment/block/record is 
corrupt?), recovery from the previous checkpoint would also stop at the latest 
checkpoint.  And we don't need to replay the WAL records between the previous 
checkpoint and the latest one, because their changes are already persisted when 
the latest checkpoint was taken.  So, the user should just do pg_resetxlog and 
start the database server when the recovery cannot find the latest checkpoint 
record and PANICs?

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

[HACKERS] [bug fix] ECPG: fails to recognize embedded parameters

[Tsunakawa, Takayuki]

Hello,

This is an actual problem that our customer hit.  In ECPG, opening a cursor 
fails which is declared as follows:

EXEC SQL DECLARE cur CURSOR FOR
SELECT oid, datname
FROM pg_database
WHERE datname LIKE 'post%' ESCAPE '\' AND datconnlimit = 
:connlimit;

sqlstate: 07001
sqlerrm.sqlerrmc: too many arguments on line 30

The cause is that next_insert() in ecpglib unconditionally skips the next 
character after the backslash.

Could you review and commit the attached patch?  I also attached the test 
program for convenience.

Regards
Takayuki Tsunakawa




ecpg_escape_string.patch
Description: ecpg_escape_string.patch


escape.ec
Description: escape.ec

-- 
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] list of credits for release notes

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Peter Eisentraut
> At the PGCon Developer Meeting it was agreed[0] to add a list of credits
> to the release notes, including everyone who was mentioned in a commit
> message.  I have now completed that list.

Thank you for your hard work.  "Daisuke Higuchi" and "Higuchi Daisuke" are the 
same person (my colleague).  Please use the former.

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] PG 10 release notes

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tom Lane
> TBH, I think that's another reason for not release-noting it.  There's no
> concrete change to point to, and so it's hard to figure out what to say.
> I'm not even very sure that we should be encouraging people to change
> existing shared_buffers settings; the experiments that Takayuki-san did
> were only on Windows 10, so we don't really know that changing that would
> be a good idea on older Windows versions.

In fact, when I ran pgbench on Windows 2008 for some other unrelated reason, I 
found larger shared buffers (4GB, 8GB) was effective, too.

I didn't know pure documentation changes are not listed on the release notes.  
But I suggest listing them (of course, excluding mere typos), because the 
documentation is also important for users as well as programs.  The 
documentation is also part of software product.  If the documented behavior and 
the actual one differs and the user is wondering which is correct, he can know 
the answer only from the release note when the mismatch is fixed.  I think the 
documentation changes are more useful for users than, for example, the 
following items:

E.1.3.11. Source Code
Improve behavior of pgindent (Piotr Stefaniak, Tom Lane)
Allow WaitLatchOrSocket() to wait for socket connection on Windows (Andres 
Freund)
Overhaul documentation build process (Alexander Lakhin)
Use XSLT to build the PostgreSQL documentation (Peter Eisentraut)


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] sync process names between ps and pg_stat_activity

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Peter Eisentraut
> > Personally, I prefer "wal writer", "wal sender" and "wal receiver"
> > that separate words as other process names.  But I don't mind leaving
> > them as they are now.
> 
> If we were to change those, that would break existing queries for
> pg_stat_activity.  That's new in PG10, so we could change it if we were
> really eager.  But it's probably not worth bothering.  Then again, there
> is pg_stat_wal_receiver.  So it's all totally inconsistent.  Not sure
> where to go.

OK, I'm comfortable with as it is now.

I made this ready for committer.  You can fix the following and commit the 
patch.  Thank you.


> >   * To achieve that, we pass "wal sender process" as username and
> > username
> 
> good catch

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 fix] PG10: libpq doesn't connect to alternative hosts when some errors occur

[Tsunakawa, Takayuki]

Hello Tom, Michael,
Robert, Noah

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
> On Thu, Sep 14, 2017 at 3:23 AM, Tsunakawa, Takayuki
> <tsunakawa.ta...@jp.fujitsu.com> wrote:
> > Sorry again, but how can we handle this?  A non-PG-developer, Tels (and
> possibly someone else, IIRC) claimed that the behavior be changed during
> the beta period.  Why should we do nothing?
> 
> Because we do not have consensus on changing it.  I've decided that you're
> right, but several other people are saying "no".  I can't just go change
> it in the face of objections.

How are things decided in cases like this?  Does the RMT team usually do some 
kind of poll?

So far, there are four proponents (Tels (non-PG-developer), David J., Robert 
and me), and two opponents (Tom and Michael).

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] Process startup infrastructure is a mess

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Andres Freund
> I think we should seriously consider doing a larger refactoring of this
> soon.  I've some ideas about what to do, but I'd welcome some thoughts on
> whether others consider this a serious problem or not, and what they think
> we should do about this, first.

Thank you for raising this.  I think so, too.  Some other things that quickly 
come to mind are:

* The signal handlers are similar in many processes and redundant.  It's not 
easy to see how the processes respond to a particular signal and what signal 
can be used for new things such as dumping the stack trace in the server log.  
Maybe we can have an array of process attributes that specify the signal 
handlers, startup/shutdown order, etc.  Then the common process management code 
handles processes based on the information in the array.

* postmaster.c is very large (more than 6,000 lines) and hard to read.

* It may be easy to forget adding initialization code in the Windows-specific 
path (SubPostmasterMaain).

* It's confusing to find out which process counts toward max_connections, 
MaxBackends, the count of auxiliary processes.  As a user, I'm sometimes 
confused about the relationship between max_connections, 
autovacuum_max_workers, max_wal_senders, max_worker_processes.

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

[HACKERS] Re: [bug fix] PG10: libpq doesn't connect to alternative hosts when some errors occur

[Tsunakawa, Takayuki]

Hello, Robert, Noah

From: Robert Haas [mailto:robertmh...@gmail.com]
> On Fri, Jul 28, 2017 at 1:30 AM, Noah Misch  wrote:
> > We've reached that period.  If anyone is going to push for a change
> > here, now is the time.  Absent such arguments, the behavior won't change.
> 
> Well, I started out believing that the current behavior was for the best,
> and then completely reversed my position and favored the OP's proposal.
> Nothing has really happened since then to change my mind, so I guess I'm
> still in that camp.  But do we have any new data points?  Have any
> beta-testers tested this and what do they think?
> The only non-developer (i.e. person not living in an ivory tower) who has
> weighed in here is Tels, who favored reversing the original decision and
> adopting Tsunakawa-san's position, and that was 2 months ago.
> 
> I am pretty reluctant to tinker with this at this late date and in the face
> of several opposing votes, but I do think that we bet on the wrong horse.

Sorry again, but how can we handle this?  A non-PG-developer, Tels (and 
possibly someone else, IIRC) claimed that the behavior be changed during the 
beta period.  Why should we do nothing?

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] PG 10 release notes

[Tsunakawa, Takayuki]

It's embarrassing to ask about such a trivial thing, but I noticed the 
following line was missing in the latest release note, which was originally in 
Bruce's website:

Remove documented restriction about using large shared buffers on Windows 
(Takayuki Tsunakawa)

Is this intended?

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] Supporting huge pages on Windows

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Ashutosh Sharma
> I have once again tested the latest patch (v14 patch) on Windows and the
> results looked fine to me. Basically I have repeated the test cases which
> I had done earlier on v8 patch. For more details, on the tests that i have
> re-executed, please refer to - [1]. Thanks.

Thanks so much.  I'm relieved to know that the patch still works.

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] [bug fix] Savepoint-related statements terminates connection

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tom Lane
> The originally reported bug is fixed.  Not making any claims about other
> bugs ...

I'm sorry I couldn't reply to you.  I've recently been in a situation where I 
can't use my time for development.  I think I'll be able to rejoin the 
community activity soon.

I confirmed your patch fixed the problem.  And the code looks perfect.  Thank 
you very much.

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] Supporting huge pages on Windows

[Tsunakawa, Takayuki]

Hi Thomas, Magnus

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Thomas Munro
> Since it only conflicts with c7b8998e because of pgindent whitespace
> movement, I applied it with "patch -p1 --ignore-whitespace" and created
> a new patch.  See attached.

Thanks, Thomas.  I've added your name in the CF entry so that your name will 
also be listed on the release note, because my patch is originally based on 
your initial try.  Please remove your name just in case you mind it.  BTW, your 
auto-reviewer looks very convenient.  Thank you again for your great work.

Magnus, it would be grateful if you could review and commit the patch while 
your memory is relatively fresh.

I've been in a situation which keeps me from doing development recently, but I 
think I can gradually rejoin the community activity soon.

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] How can I find a specific collation in pg_collation when using ICU?

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Peter Eisentraut
> There are no case-insensitive collations in PostgreSQL (yet).

That's sad news, as I expected ICU to bring its various collation features to 
PostgreSQL.  I hope it will be easy to add them.


From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Peter Geoghegan
> This is not an answer to the question you asked, but it may interest you
> to know that ICU uses JIS X 4061 for Japanese, unlike Glibc. This will give
> more useful results when sorting Japanese.
> 
> The best explanation of the difference that I can understand is here, under
> "Why do CJK strings sort incorrectly in Unicode?":
> 
> https://dev.mysql.com/doc/refman/5.5/en/faqs-cjk.html

Thanks a lot.  MysQL seems to have many collations, doesn't it?

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

[HACKERS] Re: [bug fix] PG10: libpq doesn't connect to alternative hosts when some errors occur

[Tsunakawa, Takayuki]

From: Robert Haas [mailto:robertmh...@gmail.com]
> Well, I started out believing that the current behavior was for the best,
> and then completely reversed my position and favored the OP's proposal.
> Nothing has really happened since then to change my mind, so I guess I'm
> still in that camp.  But do we have any new data points?  Have any
> beta-testers tested this and what do they think?
> The only non-developer (i.e. person not living in an ivory tower) who has
> weighed in here is Tels, who favored reversing the original decision and
> adopting Tsunakawa-san's position, and that was 2 months ago.
> 
> I am pretty reluctant to tinker with this at this late date and in the face
> of several opposing votes, but I do think that we bet on the wrong horse.

Thank you Robert and Tels.  Yes, Tels's comment sounds plausible as a 
representative real user who expects high availability.  I'm sorry to repeat 
myself, but this feature is for HA, so libpq should attempt to connect to the 
next host when it fails to establish a connection.

Who can conclude this?  I don't think that no feedback from beta users means 
satisfaction with the current behavior.

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] Huge pages support on Windows

[Tsunakawa, Takayuki]

Hello, Andrea

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Andrea caldarone
> Anyone can point me in the right direction?

Thank you for your interest.

1. Download the PostgreSQL 10 source code (which is still in development), 
which is the file named postgresql-snapshot.tar.gz from here:

https://ftp.postgresql.org/pub/snapshot/dev/

2. Get the latest patch for huge page support for Windows in the mail thread 
you saw.  The file name is win_large_pages_v13.patch.

3. Decompress the source code and apply the patch.
$ tar zxf postgresql-snapshot.tar.gz
$ cd postgresql*
$ patch -p1 < win_large_pages_v13.patch

Now you can build and install the program normally.

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] Is ECPG's SET CONNECTION really not thread-aware?

[Tsunakawa, Takayuki]

Dear Meskes,

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Meskes
> Done.

Thanks, I confirmed the commit messages.


> My standard workflow is to wait a couple days to see if everything works
> nicely before backporting. Obviously this doesn't make any sense for a doc
> patch, sigh.

I see.  That's a good idea.

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] Is ECPG's SET CONNECTION really not thread-aware?

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Meskes
> I'm pretty sure it is indeed thread-aware, although I didn't provide the
> code for this feature myself.
> 
> > So the doc seems to need fix.  The patch is attached.
> 
> Thanks, applied.

Thank you, I'm relieved.

Could you also apply it to past versions if you don't mind?  The oldest 
supported version 9.2 is already thread-aware.

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] Is ECPG's SET CONNECTION really not thread-aware?

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tsunakawa,
> Takayuki
> The following page says:
> 
> https://www.postgresql.org/docs/devel/static/ecpg-connect.html#ecpg-se
> t-connection
> 
> --
> EXEC SQL AT connection-name SELECT ...;
> 
> If your application uses multiple threads of execution, they cannot share
> a connection concurrently. You must either explicitly control access to
> the connection (using mutexes) or use a connection for each thread. If each
> thread uses its own connection, you will need to use the AT clause to specify
> which connection the thread will use.
> 
> EXEC SQL SET CONNECTION connection-name;
> 
> ...It is not thread-aware.
> --
> 
> 
> What does this mean by "not thread-aware?"  Does SET CONNECTION in one
> thread change the current connection in another thread?
> It doesn't look so, because the connection management and SQL execution
> in ECPG uses thread-specific data (TSD) to get and set the current connection,
> like this:

The ECPG code sets and gets the current connection to/from the TSD, so SET 
CONNECTION is thread-aware.  I could confirm that like this:

threadA: CONNECT AS conA
threadB: CONNECT AS conB
threadA: CONNECT AS conC
threadA & threadB: SELECT pg_backend_pid() ... each thread displays different 
pids
threadA: SET CONNECTION conA
threadA & threadB: SELECT pg_backend_pid() ... each thread displays different 
pids, with thread outputting the same pid as before

So the doc seems to need fix.  The patch is attached.

Regards
Takayuki Tsunakawa



ecpg_setconn.patch
Description: ecpg_setconn.patch

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

[HACKERS] Is ECPG's SET CONNECTION really not thread-aware?

[Tsunakawa, Takayuki]

Hello,

The following page says:

https://www.postgresql.org/docs/devel/static/ecpg-connect.html#ecpg-set-connection

--
EXEC SQL AT connection-name SELECT ...;

If your application uses multiple threads of execution, they cannot share a 
connection concurrently. You must either explicitly control access to the 
connection (using mutexes) or use a connection for each thread. If each thread 
uses its own connection, you will need to use the AT clause to specify which 
connection the thread will use.

EXEC SQL SET CONNECTION connection-name;

...It is not thread-aware.
--


What does this mean by "not thread-aware?"  Does SET CONNECTION in one thread 
change the current connection in another thread?
It doesn't look so, because the connection management and SQL execution in ECPG 
uses thread-specific data (TSD) to get and set the current connection, like 
this:


bool
ECPGsetconn(int lineno, const char *connection_name)
{
struct connection *con = ecpg_get_connection(connection_name);

if (!ecpg_init(con, connection_name, lineno))
return (false);

#ifdef ENABLE_THREAD_SAFETY
pthread_setspecific(actual_connection_key, con);
#else
actual_connection = con;
#endif
return true;
}

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: retry shm attach for windows (WAS: Re: [HACKERS] OK, so culicidae is *still* broken)

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Amit Kapila
> Yes, I also share this opinion, the shm attach failures are due to
> randomization behavior, so sleep won't help much.  So, I will change the
> patch to use 100 retries unless people have other opinions.

Perhaps I'm misunderstanding, but I thought it is not known yet whether the 
cause of the original problem is ASLR.  I remember someone referred to 
anti-virus software and something else.  I guessed that the reason Noah 
suggested 1 - 5 seconds of retry is based on the expectation that the address 
space might be freed by the anti-virus software.

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: retry shm attach for windows (WAS: Re: [HACKERS] OK, so culicidae is *still* broken)

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Noah Misch
> Ten feels low to me.  The value should be be low enough so users don't give
> up and assume a permanent hang, but there's little advantage to making it
> lower.
> I'd set it such that we give up in 1-5s on a modern Windows machine, which
> I expect implies a retry count of one hundred or more.

Then, maybe we can measure the time in each iteration and give up after a 
particular seconds.

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] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tom Lane
> I didn't look at exactly how you tried to do that, but GUCs whose values
> depend on other GUCs generally don't work well at all.

transaction_read_only and transaction_isolation depends on 
default_transaction_read_only and default_transaction_isolation respectively.  
But I feel you are concerned about something I'm not aware of.  Could you share 
your worries?  I haven't found a problem so far.


> >> * Its value is false during recovery.
> 
> [ scratches head... ]  Surely this should read as "true" during recovery?

Ouch, you are right.


> Also, what happens if the standby server goes live mid-session?

The clients will know the change of session_read_only when they do something 
that calls RecoveryInProgress().  Currently, RecoveryInProgress() seems to be 
the only place where the sessions notice the promotion, so I set 
session_read_only to the value of default_transaction_read_only there.  I think 
that there is room for discussion here.  It would be ideal for the sessions to 
notice the server promotion promptly and notify the clients of the change.  I 
have no idea to do that well.

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] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.

[Tsunakawa, Takayuki]

From: Robert Haas [mailto:robertmh...@gmail.com]
> I've already stated my position on this, which is that:
> 
> * target_session_attrs=read-only is a perfectly good new feature, but we're
> past feature freeze, so it's material for v11.
> 
> * I'm not opposed to adding a GUC_REPORT GUC of some kind, but I see no
> urgency about that either.  The feature works fine as it is.  The fact that
> it could possibly be made to work more efficiently is not a critical bug.

I see.  I'll add this in the next CF for PG 11.  I'd be glad if you could 
review the patch in PG 11 development.  Also, I'll update the PostgreSQL 10 
Open Items page that way.

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] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Craig Ringer
> On 13 April 2017 at 14:59, Tsunakawa, Takayuki
> <tsunakawa.ta...@jp.fujitsu.com> wrote:
> 
> > 2. Make transaction_read_only GUC_REPORT This is to avoid the added
> > round-trip by SHOW command.  It also benefits client apps that want to
> know when the server gets promoted?  And this may simplify the libpq code.
> > I don't understand yet why we need to provide this feature for older servers
> by using SHOW.  Those who are already using <= 9.6 in production completed
> the system or application, and their business is running.  Why would they
> want to just replace libpq and use this feature?
> 
> I think "transaction_read_only" is a bit confusing for something we're
> expecting to change under us.
> 
> To me, a "read only" xact is one created with
> 
> BEGIN READ ONLY TRANSACTION;
> 
>  which I would not expect to become read/write under me, since I
> explicitly asked for read-only.
> 
> It's more like "session read only" that we're interested in IMO.

I confirmed that the attached patch successfully provides:

* target_session_attrs=read-only
* If the server is >= 10, avoid the round-trip for SHOW transaction_read_only.

For this, I added a GUC_REPORT variable session_read_only which indicates the 
session's default read-only status.  The characteristics are:

* It cannot be changed directly by the user (postgresql.conf, SET, etc.)
* Its value is the same as default_transaction_read_only when not in recovery.
* Its value is false during recovery.

Could you include this in PG 10?  I think these are necessary as the bottom 
line to meet the average expectation of users (please don't ask me what's the 
average; the main reasons are that PostgreSQL provides hot standby, PgJDBC 
enables connection to the standby (targetServerType=slave), and PostgreSQL 
emphasizes performance.)  Ideally, I wanted to add other features of PgJDBC 
(e.g. targetServerType=preferSlave), but I thought this is the limit not to 
endanger the quality of the final release.

Regards
Takayuki Tsunakawa




libpq-fast-connect-read-only.patch
Description: libpq-fast-connect-read-only.patch

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

Re: [HACKERS] Re: [doc fix] PG10: wroing description on connect_timeout when multiple hosts are specified

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
> Committed.  I also added a slight tweak to the wording of the documentation.

Thank you, the doc looks clearer.

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] [bug fix] PG10: libpq doesn't connect to alternative hosts when some errors occur

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> >  One thing I'm worried is that people here might become more conservative
> against change once the final version is released.
> 
> Any redesign after release would finish by being a new feature, which would
> be in this case a new connection parameter or an extra option that works
> with the current parameter, say something to allow soft or hard failures
> when multiple hosts are defined.

Hmm... but I can't imagine the parameter would be very meaningful for users.

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] [bug fix] PG10: libpq doesn't connect to alternative hosts when some errors occur

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> On Thu, May 18, 2017 at 11:30 PM, Robert Haas  wrote:
> > Because why?
> 
> Because it is critical to let the user know as well *why* an error happened.
> Imagine that this feature is used with multiple nodes, all primaries. If
> a DB admin busted the credentials in one of them then all the load would
> be redirected on the other nodes, without knowing what is actually causing
> the error. Then the node where the credentials have been changed would just
> run idle, and the application would be unaware of that.

In that case, the DBA can know the authentication errors in the server log of 
the idle instance.

I'm sorry to repeat myself, but libpq connection failover is the feature for 
HA.  So I believe what to prioritize is HA.

And the documentation looks somewhat misleading.  I get the impression that 
libpq tries hosts until success regardless of failure reason: it aims for 
successful connection, not giving up early.  Who can read this as "libpq gives 
up connection under some circumstances?"


https://www.postgresql.org/docs/devel/static/libpq-connect.html
--
It is possible to specify multiple host components, each with an optional port 
component, in a single URI. A URI of the form 
postgresql://host1:port1,host2:port2,host3:port3/ is equivalent to a connection 
string of the form host=host1,host2,host3 port=port1,port2,port3. Each host 
will be tried in turn until a connection is successfully established.

...
If multiple host names are specified, each will be tried in turn in the order 
given.
--

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] [bug fix] PG10: libpq doesn't connect to alternative hosts when some errors occur

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Peter Eisentraut
> The problem is that if we decide to change the behavior mid-beta, then we'll
> only have the rest of beta to find out whether people will like the other
> behavior.
> 
> I would aim for the behavior that is most suitable for refinement in the
> future.  The current behavior seems to match that.

I think the pre-final release period is the very timing for refinement, in the 
perspective of users and PG developers as users.  One thing I'm worried is that 
people here might become more conservative against change once the final 
version is released.

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] [bug fix] PG10: libpq doesn't connect to alternative hosts when some errors occur

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tsunakawa,
> Takayuki
> Done.  I'll examine whether we can use SQLSTATE.

I tried conceivable errors during connection.  Those SQLSTATEs are as follows:

[transient error (after which you may want to try next host)]
53300 FATAL:  too many connections for role "tuna"
57P03 FATAL:  the database system is starting up

[configuration error (after which you may give up connection without other 
hosts. Really?)]
55000 FATAL:  database "template0" is not currently accepting connections
3D000 FATAL:  database "aaa" does not exist
28000 FATAL:  no pg_hba.conf entry for host "::1", user "tunakawa", database 
"postgres", SSL off
28000 FATAL:  role "nouser" does not exist
28P01 FATAL:  password authentication failed for user "tuna"
28P01 DETAIL:  Password does not match for user "tuna".


I looked through the SQLSTATEs, and thought the below ones could possibly be 
returned during connection:

https://www.postgresql.org/docs/devel/static/errcodes-appendix.html

[transient error]
Class 08 - Connection Exception
Class 40 - Transaction Rollback 
Class 53 - Insufficient Resources 
Class 54 - Program Limit Exceeded 
Class 55 - Object Not In Prerequisite State 
Class 57 - Operator Intervention 
Class 58 - System Error (errors external to PostgreSQL itself) 
Class XX - Internal Error 

[configuration error]
Class 28 - Invalid Authorization Specification 
Class 3D - Invalid Catalog Name 
Class 42 - Syntax Error or Access Rule Violation 

So, how about trying connection to the next host when the class code is neither 
28, 3D, nor 42?

Honestly, I'm not happy with this approach, for a maintenance reason that 
others are worried about.  Besides, when the connection target is not postgres 
and returns invalid data, no SQLSTATE is available.  I'm sorry to repeat 
myself, but I believe PgJDBC's approach is practically good.  If you think the 
SQLSTATE is the only way to go, I will put up with it.  It would be 
disappointing if nothing is done.

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] [bug fix] PG10: libpq doesn't connect to alternative hosts when some errors occur

[Tsunakawa, Takayuki]

From: Michael Paquier [mailto:michael.paqu...@gmail.com]
> Does JDBC use something like that to make a difference between a failure
> and a move-on-to-next-one? 

No, it just tries the next host.  See the first while loop in 
org/postgresql/jdbc/core/v3/ConnectionFactoryImpl.java.


> From maintenance point of view, this would
> require lookups each time a new SQLSTATE is added. Not sure that people
> would remember that.

Yes, I have the same concern, but I'll see if there's a good way anyway (e.g. 
whether we can simply use the class code of the SQLSTATE, which seems 
hopeless.)  I guess PgJDBC's way is practical and sensible in the end.

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] [bug fix] PG10: libpq doesn't connect to alternative hosts when some errors occur

[Tsunakawa, Takayuki]

From: Tom Lane [mailto:t...@sss.pgh.pa.us]
> Sure, but part of the point of beta testing is to get user feedback.

Yes, and I'm also proposing this in the user's point of view, which I believe 
holds true for people here.  I'm worried from my support experience that strict 
customers would complain and question the HA.


> I agree with Robert's point that major redesign of the feature on the basis
> of one complaint isn't necessarily the way to go.  Since the existing
> behavior is already out in beta1, let's wait and see if anyone else complains.
> We don't need to fix it Right This Instant.

I'm OK with considering this during beta testing.  But do you think there will 
be enough beta testers and some of them will find this kind of subtle problem?  
I'm afraid this type of problem will be detected and complained after some time 
in production...  So, I think we should address this proactively on the basis 
of good sense.

> Maybe add this to the list of open issues to reconsider mid-beta?

Done.  I'll examine whether we can use SQLSTATE.

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] [bug fix] Savepoint-related statements terminates connection

[Tsunakawa, Takayuki]

From: Michael Paquier [mailto:michael.paqu...@gmail.com]
> On Fri, Mar 31, 2017 at 9:58 PM, Ashutosh Bapat
>  wrote:
> > Then the question is why not to allow savepoints as well? For that we
> > have to fix transaction block state machine.
> 
> I agree with this argument. I have been looking at the patch, and what it
> does is definitely incorrect. Any query string including multiple queries
> sent to the server is executed as a single transaction. So, while the current
> behavior of the server is definitely incorrect for savepoints in this case,
> the proposed patch does not fix anything but actually makes things worse.
> I think that instead of failing, savepoints should be able to work properly.
> As you say cursors are handled correctly, savepoints should fall under the
> same rules.

Yes, I'm in favor of your opinion.  I'll put more thought into whether it's 
feasible with invasive 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] [bug fix] PG10: libpq doesn't connect to alternative hosts when some errors occur

[Tsunakawa, Takayuki]

Hello Robert, Tom,

Thank you for being kind enough to explain.  I think I could understand your 
concern.

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
> Who is right is a judgement call, but I don't think it's self-evident that
> users want to ignore anything and everything that might have gone wrong
> with the connection to the first server, rather than only those things which
> resemble a down server.  It seems quite possible to me that if we had defined
> it as you are proposing, somebody would now be arguing for a behavior change
> in the other direction.

Judgment call... so, I understood that it's a matter of choosing between 
helping to detect configuration errors early or service continuity.  Hmm, I'd 
like to know how other databases treat this, but I couldn't find useful 
information after some Google search.  I wonder whether I sould ask PgJDBC 
people if they know something, because they chose service continuity.


From: Tom Lane [mailto:t...@sss.pgh.pa.us]
> The bigger picture here is that we only want to fail past transient errors,
> not configuration errors.  I'm willing to err in favor of regarding doubtful
> cases as transient, but most server login rejections aren't for transient
> causes.

I got "doubtful cases" as ones such as specifying non-existent host or an 
unused port number.  In that case, the configuration error can't be 
distinguished from the server failure.

What do you think of the following cases?  Don't you want to connect to other 
servers?

* The DBA shuts down the database.  The server takes a long time to do 
checkpointing.  During the shutdown checkpoint, libpq tries to connect to the 
server and receive an error "the database system is shutting down."

* The former primary failed and now is trying to start as a standby, catching 
up by applying WAL.  During the recovery, libpq tries to connect to the server 
and receive an error "the database system is performing recovery."

* The database server crashed due to a bug.  Unfortunately, the server takes 
unexpectedly long time to shut down because it takes many seconds to write the 
stats file (as you remember, Tom-san experienced 57 seconds to write the stats 
file during regression tests.)  During the stats file write, libpq tries to 
connect to the server and receive an error "the database system is shutting 
down."

These are equivalent to server failure.  I believe we should prioritize 
rescuing errors during operation over detecting configuration errors.


> Of course, the user would have to try connections to both foo and bar to
> be sure that they're both configured correctly.  But he might try
> "host=foo,bar" and "host=bar,foo" and figure he was OK, not noticing that
> both connections had silently been made to bar.

In that case, I think he would specify "host=foo" and "host=bar" in turn, 
because he would be worried about where he's connected if he specified multiple 
hosts.

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: [doc fix] PG10: wroing description on connect_timeout when multiple hosts are specified

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> pqWait is internal to libpq, so we are free to set up what we want here.
> Still I think that we should be consistent with what pqSocketCheck returns:

Please let this what it is now for the same reason Robert mentioned.

> +intret = 0;
> +inttimeout = 0;
> The declaration of "ret" should be internal in the for(;;) loop.

Done.


> +   /* Attempt connection to the next host, starting the
> connect_timeout timer */
> +   pqDropConnection(conn, true);
> +   conn->addr_cur = conn->connhost[conn->whichhost].addrlist;
> +   conn->status = CONNECTION_NEEDED;
> +   finish_time = time(NULL) + timeout;
> +   }
> I think that it would be safer to not set finish_time if
> conn->connect_timeout is NULL. I agree that your code works because
> pqWaitTimed() will never complain on timeout reached if finish_time is -1.
> That's for robustness sake.

Done, but I'm not sure how this contributes to the robustness.  I guess you 
were concerned just in case pqWaitTimed() returned 0 (timeout) even when it 
should not.

Regards
Takayuki Tsunakawa




libpq-retry-connect-on-timeout_v2.patch
Description: libpq-retry-connect-on-timeout_v2.patch

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

Re: [HACKERS] Re: [doc fix] PG10: wroing description on connect_timeout when multiple hosts are specified

[Tsunakawa, Takayuki]

Hello Robert, Tom, Michael,

Thanks a lot for checking my patch.  Sorry, let me check Michael's review 
comments and reply tomorrow.

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] [bug fix] PG10: libpq doesn't connect to alternative hosts when some errors occur

[Tsunakawa, Takayuki]

From: Michael Paquier [mailto:michael.paqu...@gmail.com]
> On Fri, May 12, 2017 at 10:44 PM, Tom Lane  wrote:
> > I would not really expect that reconnection would retry after
> > arbitrary failure cases.  Should it retry for "wrong database name", for
> instance?
> > It's not hard to imagine that leading to very confusing behavior.
> 
> I guess not as well. That would be tricky for the user to have a different
> behavior depending on the error returned by the server, which is why the
> current code is doing things right IMO. Now, the feature has been designed
> similarly to JDBC with its parametrization, so it could be surprising for
> users to get a different failure handling compared to that. Not saying that
> JDBC is doing it wrong, but libpq does nothing wrong either.

I didn't intend to make the user have a different behavior depending on the 
error returned by the server.  I meant attempting connection to alternative 
hosts when the server returned an error. I thought the new libpq feature tries 
to connect to other hosts when a connection attempt fails, where the 
"connection" is the *database connection* (user's perspective), not the *socket 
connection* (PG developer's perspective).  I think PgJDBC meets the user's 
desire better -- "Please connect to some host for better HA if a database 
server is unavailable for some reason."

By the way, could you elaborate what problem could occur if my solution is 
applied?  (it doesn't seem easy for me to imagine...)  FYI, as below, the case 
Tom picked up didn't raise an issue:

[libpq]
$ psql -h localhost,localhost -p 5450,5451 -d aaa
psql: FATAL:  database "aaa" does not exist
$


[JDBC]
$ java org.hsqldb.cmdline.SqlTool postgres
SqlTool v. 3481.
2017-05-15T10:23:55.991+0900  SEVERE  Connection error:
org.postgresql.util.PSQLException: FATAL: database "aaa" does not exist
  Location: File: postinit.c, Routine: InitPostgres, Line: 846
  Server SQLState: 3D000
at 
org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2412)
at 
org.postgresql.core.v3.QueryExecutorImpl.readStartupMessages(QueryExecutorImpl.java:2538)
at 
org.postgresql.core.v3.QueryExecutorImpl.(QueryExecutorImpl.java:122)
at 
org.postgresql.core.v3.ConnectionFactoryImpl.openConnectionImpl(ConnectionFactoryImpl.java:227)
at 
org.postgresql.core.ConnectionFactory.openConnection(ConnectionFactory.java:49)
at org.postgresql.jdbc.PgConnection.(PgConnection.java:194)
at org.postgresql.Driver.makeConnection(Driver.java:431)
at org.postgresql.Driver.connect(Driver.java:247)
at java.sql.DriverManager.getConnection(DriverManager.java:664)
at java.sql.DriverManager.getConnection(DriverManager.java:247)
at org.hsqldb.lib.RCData.getConnection(Unknown Source)
at org.hsqldb.cmdline.SqlTool.objectMain(Unknown Source)
at org.hsqldb.cmdline.SqlTool.main(Unknown Source)

Failed to get a connection to 
'jdbc:postgresql://localhost:5450,localhost:5451/aaa' as user "tunakawa".
Cause: FATAL: database "aaa" does not exist
  Location: File: postinit.c, Routine: InitPostgres, Line: 846
  Server SQLState: 3D000
$

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

[HACKERS] Re: [doc fix] PG10: wroing description on connect_timeout when multiple hosts are specified

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tsunakawa,> Takayuki
> Instead, I think we should fix the program to match the documented behavior.
> Otherwise, if the first database machine is down, libpq might wait for about
> 2 hours (depending on the OS's TCP keepalive setting), during which it tims
> out after connect_timeout and does not attempt to connect to other hosts.
> 
> I'll add this item in the PostgreSQL 10 Open Items.

Please use the attached patch to fix the problem.  I confirmed the success as 
follows:

$ add "post_auth_delay = 10" in postgresql.conf on host1
$ start database servers on host1 and host2
$ psql -h host1,host2 -p 5432,5433 -d "dbname=postgres connect_timeout=3"
(psql connected to host2 after 3 seconds, which I checked with \conninfo)

Regards
Takayuki Tsunakawa

diff --git a/src/interfaces/libpq/fe-connect.c 
b/src/interfaces/libpq/fe-connect.c
index eb5aaf7098..d02e5201fa 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1720,6 +1720,8 @@ connectDBComplete(PGconn *conn)
 {
PostgresPollingStatusType flag = PGRES_POLLING_WRITING;
time_t  finish_time = ((time_t) -1);
+   int ret = 0;
+   int timeout = 0;
 
if (conn == NULL || conn->status == CONNECTION_BAD)
return 0;
@@ -1729,8 +1731,7 @@ connectDBComplete(PGconn *conn)
 */
if (conn->connect_timeout != NULL)
{
-   int timeout = atoi(conn->connect_timeout);
-
+   timeout = atoi(conn->connect_timeout);
if (timeout > 0)
{
/*
@@ -1761,7 +1762,8 @@ connectDBComplete(PGconn *conn)
return 1;   /* success! */
 
case PGRES_POLLING_READING:
-   if (pqWaitTimed(1, 0, conn, finish_time))
+   ret = pqWaitTimed(1, 0, conn, finish_time);
+   if (ret == -1)
{
conn->status = CONNECTION_BAD;
return 0;
@@ -1769,7 +1771,8 @@ connectDBComplete(PGconn *conn)
break;
 
case PGRES_POLLING_WRITING:
-   if (pqWaitTimed(0, 1, conn, finish_time))
+   ret = pqWaitTimed(0, 1, conn, finish_time);
+   if (ret == -1)
{
conn->status = CONNECTION_BAD;
return 0;
@@ -1782,6 +1785,22 @@ connectDBComplete(PGconn *conn)
return 0;
}
 
+   if (ret == 1)   /* connect_timeout elapsed */
+   {
+   /* If there are no more hosts, return (the error 
message is already set) */
+   if (++conn->whichhost >= conn->nconnhost)
+   {
+   conn->whichhost = 0;
+   conn->status = CONNECTION_BAD;
+   return 0;
+   }
+   /* Attempt connection to the next host, starting the 
connect_timeout timer */
+   pqDropConnection(conn, true);
+   conn->addr_cur = 
conn->connhost[conn->whichhost].addrlist;
+   conn->status = CONNECTION_NEEDED;
+   finish_time = time(NULL) + timeout;
+   }
+
/*
 * Now try to advance the state machine.
 */
diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index 756c6d7779..1d6ea93a0a 100644
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -991,11 +991,9 @@ pqWait(int forRead, int forWrite, PGconn *conn)
 /*
  * pqWaitTimed: wait, but not past finish_time.
  *
- * If finish_time is exceeded then we return failure (EOF).  This is like
- * the response for a kernel exception because we don't want the caller
- * to try to read/write in that case.
- *
  * finish_time = ((time_t) -1) disables the wait limit.
+ *
+ * Returns -1 on failure, 0 if the socket is readable/writable, 1 if it timed 
out.
  */
 int
 pqWaitTimed(int forRead, int forWrite, PGconn *conn, time_t finish_time)
@@ -1005,13 +1003,13 @@ pqWaitTimed(int forRead, int forWrite, PGconn *conn, 
time_t finish_time)
result = pqSocketCheck(conn, forRead, forWrite, finish_time);
 
if (result < 0)
-   return EOF; /* errorMessage is 
already set */
+   return -1;  /* errorMe

[HACKERS] Re: [doc fix] PG10: wroing description on connect_timeout when multiple hosts are specified

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tsunakawa,
> Takayuki
> I found a wrong sentence here in the doc.  I'm sorry, this is what I asked
> you to add...
> 
> https://www.postgresql.org/docs/devel/static/libpq-connect.html#libpq-
> paramkeywords
> 
> connect_timeout
> Maximum wait for connection, in seconds (write as a decimal integer string).
> Zero or not specified means wait indefinitely. It is not recommended to
> use a timeout of less than 2 seconds. This timeout applies separately to
> each connection attempt. For example, if you specify two hosts and both
> of them are unreachable, and connect_timeout is 5, the total time spent
> waiting for a connection might be up to 10 seconds.
> 
> 
> The program behavior is that libpq times out after connect_timeout seconds
> regardless of how many hosts are specified.  I confirmed it like this:
> 
> $ export PGOPTIONS="-c post_auth_delay=30"
> $ psql -d "dbname=postgres connect_timeout=5" -h localhost,localhost -p
> 5432,5433
> (psql erros out after 5 seconds)
> 
> Could you fix the doc with something like this?
> 
> "This timeout applies across all the connection attempts. For example, if
> you specify two hosts and both of them are unreachable, and connect_timeout
> is 5, the total time spent waiting for a connection is up to 5 seconds."
> 
> Should we also change the minimum "2 seconds" part to be longer, according
> to the number of hosts?

Instead, I think we should fix the program to match the documented behavior.  
Otherwise, if the first database machine is down, libpq might wait for about 2 
hours (depending on the OS's TCP keepalive setting), during which it tims out 
after connect_timeout and does not attempt to connect to other hosts.

I'll add this item in the PostgreSQL 10 Open Items.

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] [bug fix] PG10: libpq doesn't connect to alternative hosts when some errors occur

[Tsunakawa, Takayuki]

From: Michael Paquier [mailto:michael.paqu...@gmail.com]
> It seems to me that the feature is behaving as wanted. Or in short attempt
> to connect to the next host only if a connection cannot be established.
> If there is a failure once the exchange with the server has begun, just
> consider it as a hard failure. This is an important property for
> authentication and SSL connection failures actually.

But PgJDBC behaves as expected -- attempt another connection to other hosts 
(and succeed).  I believe that's what users would naturally expect.  The 
current libpq implementation handles only the socket-level connect failure.

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

[HACKERS] [bug fix] PG10: libpq doesn't connect to alternative hosts when some errors occur

[Tsunakawa, Takayuki]

Hello,

I found a problem with libpq connection failover.  When libpq cannot connect to 
earlier hosts in the host list, it doesn't try to connect to other hosts.  For 
example, when you specify a wrong port that some non-postgres program is using, 
or some non-postgres program is using PG's port unexpectedly, you get an error 
like this:

$ psql -h localhost -p 23
psql: received invalid response to SSL negotiation: 
$ psql -h localhost -p 23 -d "sslmode=disable"
psql: expected authentication request from server, but received 

Likewise, when the first host has already reached max_connections, libpq 
doesn't attempt the connection aginst later hosts.

The attached patch fixes this.  I'll add this item in the PostgreSQL 10 Open 
Items.


Regards
Takayuki Tsunakawa



libpq-reconnect-on-error.patch
Description: libpq-reconnect-on-error.patch

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

[HACKERS] [doc fix] PG10: wroing description on connect_timeout when multiple hosts are specified

[Tsunakawa, Takayuki]

Hello, Robert

I found a wrong sentence here in the doc.  I'm sorry, this is what I asked you 
to add...

https://www.postgresql.org/docs/devel/static/libpq-connect.html#libpq-paramkeywords

connect_timeout
Maximum wait for connection, in seconds (write as a decimal integer string). 
Zero or not specified means wait indefinitely. It is not recommended to use a 
timeout of less than 2 seconds. This timeout applies separately to each 
connection attempt. For example, if you specify two hosts and both of them are 
unreachable, and connect_timeout is 5, the total time spent waiting for a 
connection might be up to 10 seconds.


The program behavior is that libpq times out after connect_timeout seconds 
regardless of how many hosts are specified.  I confirmed it like this:

$ export PGOPTIONS="-c post_auth_delay=30"
$ psql -d "dbname=postgres connect_timeout=5" -h localhost,localhost -p 
5432,5433
(psql erros out after 5 seconds)

Could you fix the doc with something like this?

"This timeout applies across all the connection attempts. For example, if you 
specify two hosts and both of them are unreachable, and connect_timeout is 5, 
the total time spent waiting for a connection is up to 5 seconds."

Should we also change the minimum "2 seconds" part to be longer, according to 
the number of hosts?

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] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Craig Ringer
> On 13 April 2017 at 14:59, Tsunakawa, Takayuki
> <tsunakawa.ta...@jp.fujitsu.com> wrote:
> 
> > 2. Make transaction_read_only GUC_REPORT This is to avoid the added
> > round-trip by SHOW command.  It also benefits client apps that want to
> know when the server gets promoted?  And this may simplify the libpq code.
> > I don't understand yet why we need to provide this feature for older servers
> by using SHOW.  Those who are already using <= 9.6 in production completed
> the system or application, and their business is running.  Why would they
> want to just replace libpq and use this feature?
> 
> I think "transaction_read_only" is a bit confusing for something we're
> expecting to change under us.
> 
> To me, a "read only" xact is one created with
> 
> BEGIN READ ONLY TRANSACTION;
> 
>  which I would not expect to become read/write under me, since I
> explicitly asked for read-only.
> 
> It's more like "session read only" that we're interested in IMO.

Are you suggest thating we provide a GUC_REPORT read-only variable 
"session_read_only" and the libpq should use it?

Anyway, I added this item in the PostgreSQL 10 Open Items page under
"Design Decisions to Recheck Mid-Beta".  I'm willing to submit a patch for PG10.

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] [PostgreSQL 10] default of hot_standby should be "on"?

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Masahiko Sawada
> The idea of changing the default value seems good to me but I'm not sure
> it's good idea to change the default value now under the circumstances where
> we're focus on stabilization.
> Also we should update the document as well.
> 

We can consider like this: the OP found a usability problem as a result of PG 
10 development, so we will fix it as a stabilization work.

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] PG 10 release notes

[Tsunakawa, Takayuki]

From: 'Bruce Momjian' [mailto:br...@momjian.us]
> > I forgot to point out one thing.
> >
> > Allow libpq to connect to multiple specified host names (Robert Haas)
> > libpq will connect with the first responsive host name.
> >
> > According to the following CF entry and my memory,
> >
> > https://commitfest.postgresql.org/12/879/
> >
> > Authors
> > mithun cy (mithun.cy)
> >
> > Reviewers
> > Gerdan Santos (gerdan), Takayuki Tsunakawa (maumau)Remove from
> > reviewers
> >
> > Committer
> > Robert Haas (rhaas)
> >
> > The author should probably be "(Mithun Cy, Robert Haas, Victor Wagner)"
> 
> I based the release note text on this commit:
> 
>   commit 274bb2b3857cc987cfa21d14775cae9b0dababa5
>   Author: Robert Haas 
>   Date:   Thu Nov 3 09:25:20 2016 -0400
> 
>   libpq: Allow connection strings and URIs to specify multiple
> hosts.
> 
>   It's also possible to specify a separate port for each host.
> 
>   Previously, we'd loop over every address returned by looking
> up the
>   host name; now, we'll try every address for every host name.
> 
>   Patch by me.  Victor Wagner wrote an earlier patch for this
> feature,
>   which I read, but I didn't use any of his code.  Review by Mithun
> Cy.
> 
> Was it wrong?

Oh, that commit.  I got it.

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] Cached plans and statement generalization

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Konstantin
> Knizhnik
> Well, first of all I want to share results I already get: pgbench with default
> parameters,  scale 10 and one connection:
> 
> So autoprepare is as efficient as explicit prepare and can increase
> performance almost two times.

This sounds great.

BTW, when are the autoprepared statements destroyed?  Are you considering some 
upper limit on the number of prepared statements?

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] PG 10 release notes

[Tsunakawa, Takayuki]

Hello, Bruce

I forgot to point out one thing.

Allow libpq to connect to multiple specified host names (Robert Haas) 
libpq will connect with the first responsive host name. 

According to the following CF entry and my memory,

https://commitfest.postgresql.org/12/879/

Authors
mithun cy (mithun.cy) 

Reviewers
Gerdan Santos (gerdan), Takayuki Tsunakawa (maumau)Remove from reviewers 

Committer
Robert Haas (rhaas)  

The author should probably be "(Mithun Cy, Robert Haas, Victor Wagner)"

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] PG 10 release notes

[Tsunakawa, Takayuki]

Hello, Bruce

> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Bruce Momjian
> I have committed the first draft of the Postgres 10 release notes.  They
> are current as of two days ago, and I will keep them current.  Please give
> me any feedback you have.


Thanks for a nice release note.  Below is what I noticed:


(1)
Support parallel bitmap heap scans (Dilip Kum)

This item appears twice.


(2)
E.1.3.1.4. Optimizer
Remove SCO and Unixware ports (Tom Lane)

The section doesn't seem appropriate.  Is OS-specific code related to the 
optimizer?



(3)
Remove documented restriction about using large shared buffers on Windows 
(Tsunakawa, Takayuki)

Please change the name to "Takayuki Tsunakawa".


(4)
Have to_timestamp() and to_date() check check input values for validity (Artur 
Zakirov)

"check" is repeated.


(5)
Use POSIX semaphores rather than SysV semaphores on Linux and FreeBSD (Tom Lane)
This avoids some limits on SysV semaphores usage.

These two lines are put in two different items.


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] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.

[Tsunakawa, Takayuki]

Hello,

I didn't realize that my target_session_attrs naming proposal was committed.  I 
didn't think half way it would be adopted, because the name is less intuitive 
than the original target_server_type, and is different from the PgJDBC's 
targetServerType.


From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Simon Riggs
> I agree if we introduce target_session_attrs it would be better to have
> a complete feature in one release.
> 
> It does seem quite strange to have
>   target_session_attrs=read-write
> but not
>   target_session_attrs=read-only

I totally agree.  I'm a bit disappointed with and worried about the current 
situation.  I could easily imagine that people around me would say a stern 
opinion on the specification...

I think these are necessary in descending order of priority, if based on 
target_session_attrs:

[PG10]
1. target_session_attrs=read-only
This is mainly to connect to the standby.  People will naturally expect that 
this is available, because PostgreSQL provides hot standby feature, and other 
DBMSs and even PgJDBC has the functionality.

2. Make transaction_read_only GUC_REPORT
This is to avoid the added round-trip by SHOW command.  It also benefits client 
apps that want to know when the server gets promoted?  And this may simplify 
the libpq code.
I don't understand yet why we need to provide this feature for older servers by 
using SHOW.  Those who are already using <= 9.6 in production completed the 
system or application, and their business is running.  Why would they want to 
just replace libpq and use this feature?

[PG 11]
3. target_session_attrs=prefer-read-only
This is mainly to prefer standbys, but allows to connect to the primary if no 
standby is available.  Honestly, this is also required in PG 10 because PgJDBC 
already provides this by "preferSlave".



From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
> On Tue, Apr 11, 2017 at 4:05 AM, Magnus Hagander 
> wrote:
> > The part I'm talking about is the potential adjustment of the patch
> > that's already committed. That's not a new feature, that's exactly the
> > sort of thing we'd want to adjust before we get to release. Because
> > once released we really can't change it.
> 
> I don't really agree.  I think if we go and install a GUC_REPORT GUC now,
> we're much less likely to flush out the bugs in the 'show
> transaction_read_only' mechanism.  

I'm sorry I couldn't get this part (maybe purely English nuance.  Are you 
concerned about some bugs?  We can't do anything if we fear of bugs.  Is it OK 
instead to make transaction_read_only GUC_REPORT?


> Also, now that I think about, the reason
> why we settled on 'show transaction_read_only' against alternate queries
> is because there's some ability for the DBA to make that return 'false'
> rather than 'true' even when not in recovery, so that if for example you
> are using logical replication rather than physical replication, you have
> a way to make target_session_attrs=read-write still do something useful.
> If you add an in_hot_standby GUC that's used instead, you lose that.

Agreed.  Again, is this satisfied by GUC_REPORTing transaction_read_only as 
well?


> Now, we can decide what we want to do about that, but I don't see that a
> change in this area *must* go into v10.  Maybe the answer is that
> target_session_attrs grows additional values like 'primary' and 'standby'
> alongside 'read-write' (and Simon's suggested 'read-only').
> Or maybe we have another idea.  But I don't really see the urgency of
> whacking this around right this minute.  There's nothing broken here;
> there's just more stuff people would like to have.  It can be added next
> time around.

But if completeness of the functionality is below people's expectations, it may 
unnecessarily compromise the reputation of PostgreSQL.

Is there any chance to incorporate a patch into PG 10?  May I add this as a PG 
10 open item?


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] Supporting huge pages on Windows

[Tsunakawa, Takayuki]

Hello, Magnus
cc: Andres

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Magnus Hagander
> I think what you'd need to do is enumerate what privileges the user has
> *before* calling CreateRestrictedToken(), using GetTokenInformation().
> And then pass those into PrivilegesToDelete (except for
> SeChangeNotifyPrivilege) in the call to CreateRestrictedToken(), instead
> of using DISABLE_MAX_PRIVILEGE. (and add the privilege needed for huge pages
> before you start that whole process -- that needs to be added in the token
> used *before* we create the restricted one).
> 
> At least that's my guess from reading the docs and trying to remember :)

Oh, I got it now.  Thanks.  The revised patch is attached.  The only modified 
file is pg_ctl.c.  The patch worked as expected.

It is regrettable that I could not make it in time for PG 10, but I'd 
appreciate it if you could review and commit this patch early in PG 11 while 
our memory is fresh.  Thank you for your patience.  I'll create an entry in the 
next CF soon.

Regards
Takayuki Tsunakawa





win_large_pages_v13.patch
Description: win_large_pages_v13.patch

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

Re: [HACKERS] Supporting huge pages on Windows

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Andres Freund
> As I asked before, why can't we delete all privs and add the explicitly
> needed once back (using AdjustTokenPrivileges)?

I tried it with pg_ctl.c attached to an earlier mail today, i.e. delete all 
privs with CreateRestrictedToken(DISABLE_ALL_PRIVILEGE) and enable Lock Pages 
in Memory with AdjustTokenPrivileges().  But it didn't work; 
AdjustTokenPrivileges() failed to enable the priv.  It's probably that 
CreateRestrictedToken() deletes (unassigns?) the privs from the access token, 
so subsequent AdjustTokenPrivileges() can no longer enable the priv.

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] Statement timeout behavior in extended queries

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tatsuo Ishii
> Since pgproto is a dumb protocol machine, it does not start a transaction
> automatically (user needs to explicitly send a start transaction command
> via either simple or extended query). In this particular case no explicit
> transaction has started.
> 

Then, the following sequence should have occurred.  The test result is valid.

# Execute statement which takes 2 seconds.
'P' "S1""SELECT pg_sleep(2)"0
  -> start transaction T1
'B' "S2""S1"0   0   0

'P' ""  "SET statement_timeout = '1s'"  0
'B' ""  ""  0   0   0
'E' ""  0

# Execute statement which takes 2 seconds (statement timeout expected).
'E' "S2"0
  -> timeout error occurred, T1 aborted

# Issue Sync message
'S'
  -> rolled back T1, so statement_timeout reverted to 3s

# Receive response from backend
'Y'

# Execute statement which takes 2 seconds (statement timeout expected).
'P' "S3""SELECT pg_sleep(2)"0
  -> start transaction T2
'B' "S2""S3"0   0   0
'E' "S2"0

# Issue Sync message
'S'
  -> committed T2

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] Statement timeout behavior in extended queries

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tatsuo Ishii
> I have done tests using pgproto. One thing I noticed a strange behavior.
> Below is an output of pgproto. The test first set the timeout to 3 seconds,
> and parse/bind for "SELECT pg_sleep(2)" then set timeout to 1 second using
> extended query. Subsequent Execute emits a statement timeout error as
> expected, but next "SELECT pg_sleep(2)"
> call using extended query does not emit a statement error. The test for
> this is "007-timeout-twice". Attached is the test cases including this.

What's the handling of transactions like in pgproto?  I guess the first 
statement timeout error rolled back the effect of "SET statement_timeout = 
'1s'", and the timeout reverted to 3s or some other value.

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] Statement timeout behavior in extended queries

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of 'Andres Freund'
> Attached.  I did not like that the previous patch had the timeout handling
> duplicated in the individual functions, I instead centralized it into
> start_xact_command().  Given that it already activated the timeout in the
> most common cases, that seems to make more sense to me. In your version
> we'd have called enable_statement_timeout() twice consecutively (which
> behaviourally is harmless).
> 
> What do you think?  I've not really tested this with the extended protocol,
> so I'd appreciate if you could rerun your test from the older thread.

The patch looks good and cleaner.  It looks like the code works as expected.  
As before, I ran one INSERT statement with PgJDBC, with gdb's breakpoints set 
on enable_timeout() and disable_timeout().  I confirmed that enable_timeout() 
is called just once at Parse message, and disable_timeout() is called just once 
at Execute message.

I'd like to wait for Tatsuo-san's thorough testing with pgproto.

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] Supporting huge pages on Windows

[Tsunakawa, Takayuki]

From: Craig Ringer [mailto:craig.rin...@2ndquadrant.com]
> On 5 April 2017 at 10:37, Tsunakawa, Takayuki
> <tsunakawa.ta...@jp.fujitsu.com> wrote:
> 
> OTOH, I tried again to leave the DISABLE_MAX_PRIVILEGE as is and add Lock
> Pages in Memory, using the attached pg_ctl.c.  Please see
> EnableLockPagesPrivilege() and its call site.  But pg_ctl -w start fails
> emitting the following message:
> 
> That won't work. You'd have to pass 0 to the flags of CreateRestrictedToken
> and instead supply a PrivilegesToDelete array.
> You'd probably GetTokenInformation and AND with a mask of ones you wanted
> to retain.

Uh, that's inconvenient.  We can't determine what privileges to delete, and we 
must be aware of new privileges added in the future version of Windows.

Then, I have to say the last patch (v12) is the final answer.

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] Supporting huge pages on Windows

[Tsunakawa, Takayuki]

From: Craig Ringer [mailto:craig.rin...@2ndquadrant.com]
> TBH, anyone who cares about security and runs Win7 or Win2k8 or newer should
> be using virtual service accounts and managed service accounts.
> 
> https://technet.microsoft.com/en-us/library/dd548356
> 
> 
> Those are more like Unix service accounts. Notably they don't need a password,
> getting rid of some of the management pain that led us to abandon the
> 'postgres' system user on Windows.
> 
> Now that older platforms are EoL and even the oldest that added this feature
> are also near EoL or in extended maintenance, I think installers should
> switch to these by default instead of using NETWORK SERVICE.
> 
> Then the issue of priv dropping would be a lesser concern anyway.

Good point!  And I said earlier in this thread, I think managing privileges 
(adding/revoking privileges from the user account) is the DBA's or sysadmin's 
duty, and PG's removing all privileges feels overkill.

OTOH, I tried again to leave the DISABLE_MAX_PRIVILEGE as is and add Lock Pages 
in Memory, using the attached pg_ctl.c.  Please see EnableLockPagesPrivilege() 
and its call site.  But pg_ctl -w start fails emitting the following message:

error code 1300
waiting for server to startFATAL:  could not enable "Lock pages in memory" 
privilege
HINT:  Assign "Lock pages in memory" privilege to the Windows user account 
which runs PostgreSQL.
LOG:  database system is shut down
 stopped waiting
pg_ctl: could not start server
Examine the log output.

error code 1300 is ERROR_NOT_ALL_ASSIGNED, which means AdjustTokenPrivileges() 
cound not enable Lock Pages in Memory privilege.  It seems that the privilege 
cannot be enabled once it was removed with 
CreateRestrictedToken(DISABLE_MAX_PRIVILEGE).

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 

Re: [HACKERS] Statement timeout behavior in extended queries

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tatsuo Ishii
> Hmm. IMO, that could happen even with the current statement timeout
> implementation as well.
> 
> Or we could start/stop the timeout in exec_execute_message() only. This
> could avoid the problem above. Also this is more consistent with
> log_duration/log_min_duration_statement behavior than now.

I think it's better to include Parse and Bind as now.  Parse or Bind could take 
long time when the table has many partitions, the query is complex and/or very 
long, some DDL statement is running against a target table, or the system load 
is high.

Firing statement timeout during or after many Parses is not a problem, because 
the first Parse started running some statement and it's not finished.  Plus, 
Andres confirmed that major client drivers don't use such a pattern.  There may 
be an odd behavior purely from the perspective of E.Q.P, that's a compromise, 
which Andres meant by "not perfect but."

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] Statement timeout behavior in extended queries

[Tsunakawa, Takayuki]

From: Andres Freund [mailto:and...@anarazel.de]
> Looks to me like npgsql doesn't do that either.  None of libpq, pgjdbs and
> npgsql doing it seems like some evidence that it's ok.

And psqlODBC now uses always libpq.

Now time for final decision?

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] Statement timeout behavior in extended queries

[Tsunakawa, Takayuki]

From: Andres Freund [mailto:and...@anarazel.de]
Given the concern raised in
> http://archives.postgresql.org/message-id/12207.1491228316%40sss.pgh.p
> a.us
> I don't see this being ready for committer.

If what Tatsuo-san said to Tom is correct (i.e. each Parse/Bind/Execute starts 
and stops the timer), then it's a concern and the patch should not be ready for 
committer.  However, the current patch is not like that -- it seems to do what 
others in this thread are expecting.

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] Statement timeout behavior in extended queries

[Tsunakawa, Takayuki]

From: Tatsuo Ishii [mailto:is...@sraoss.co.jp]
> It's too late. Someone has already moved the patch to the next CF (for
> PostgreSQL 11).

Yes, but this patch will be necessary by the final release of PG 10 if the 
libpq batch/pipelining is committed in PG 10.

I marked this as ready for committer in the next CF, so that some committer can 
pick up this patch and consider putting it in PG 10.  If you decide to modify 
the patch, please change the status.

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] Supporting huge pages on Windows

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Andres Freund
> I don't think the errdetail is quite right - OpenProcessToken isn't really
> a syscall, is it? But then it's a common pattern already in wind32_shmem.c...

Yes, "Win32 API function" would be correct, but I followed the existing code...


> > +errdetail("Failed system call was %s.",
> > +"LookupPrivilegeValue")));
> 
> Other places in the file actually log the arguments to the function...

The only place is CreateFileMapping.  Other places (DuplicateHandle and 
MapViewOfFileEx) don't log arguments.  I guess the original developer thought 
that size and name arguments to CreateFileMapping() might be useful for 
troubleshooting.


> Wonder if we should quote "Lock Pages in Memory" or add dashes, to make
> sure it's clear that that's the right?

I saw several Microsoft pages, including a page someone introduced me here, and 
they didn't quote the user right.  I'm comfortable with the current code, but I 
don't mind if the committer adds some quotation.


> > +   flProtect = PAGE_READWRITE | SEC_COMMIT |
> SEC_LARGE_PAGES;
> 
> Why don't we just add the relevant flag, instead of overwriting the previous
> contents?

I don't have strong opinion on this...

> Uh - isn't that a behavioural change?  Was this discussed?

Yes, I described this in the first mail.  Magnus asked about this later and I 
replied.

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] Statement timeout behavior in extended queries

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tatsuo Ishii
> Actually the statement timer is replaced with new statement timer value
> in enable_statement_timeout().

The patch doesn't seem to behave like that.  The Parse message calls 
start_xact_command() -> enable_statement_timeout() -> enable_timeout(), and set 
stmt_timer_started to true.  Subsequent Bind and Execute messages call 
enable_statement_timeout(), but enable_statement_timeout() doesn't call 
enable_timeout() because stmt_timer_started is already true.


> > It looks like the patch does the following.  I think this is desirable,
> because starting and stopping the timer for each message may be costly as
> Tom said.
> > Parse(statement1)
> >   start timer
> > Bind(statement1, portal1)
> > Execute(portal1)
> >   stop timer
> > Sync

I ran one INSERT statement using JDBC with log_min_messages = DEBUG3, and the 
server log shows what I said.

DEBUG:  parse : insert into a values(2)
DEBUG:  Set statement timeout
LOG:  duration: 0.431 ms  parse : insert into a values(2)
DEBUG:  bind  to 
LOG:  duration: 0.127 ms  bind : insert into a values(2)
DEBUG:  Disable statement timeout
LOG:  duration: 0.184 ms  execute : insert into a values(2)
DEBUG:  snapshot of 1+0 running transaction ids (lsn 0/163BF28 oldest xid 561 
latest complete 560 next xid 562)


> This doesn't work in general use cases. Following pattern appears frequently
> in applications.
> 
> Parse(statement1)
> Bind(statement1, portal1)
> Execute(portal1)
> Bind(statement1, portal1)
> Execute(portal1)
> :
> :
> Sync

It works.  The first Parse-Bind-Execute is measured as one unit, then 
subsequent Bind-Execute pairs are measured as other units.  That's because each 
Execute ends the statement_timeout timer and the Bind message starts it again.  
I think this is desirable, so the current patch looks good.  May I mark this as 
ready for committer?  FYI, make check-world passed successfully.


> Also what would happen if client just send a parse message and does nothing
> after that?

It's correct to trigger the statement timeout in this case, because the first 
SQL statement started (with the Parse message) and its execution is not 
finished (with Execute message.)


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] Statement timeout behavior in extended queries

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tatsuo Ishii
> No. Parse, bind and Execute message indivually stops and starts the
> statement_timeout timer with the patch. Unless there's a lock conflict,
> parse and bind will take very short time. So actually users could only
> observe the timeout in execute message though.

Where is the statement_timeout timer stopped when processing Parse and Bind 
messages?  Do you mean the following sequence of operations are performed in 
this patch?

Parse(statement1)
  start timer
  stop timer
Bind(statement1, portal1)
  start timer
  stop timer
Execute(portal1)
  start timer
  stop timer
Sync

It looks like the patch does the following.  I think this is desirable, because 
starting and stopping the timer for each message may be costly as Tom said.

Parse(statement1)
  start timer
Bind(statement1, portal1)
Execute(portal1)
  stop timer
Sync


> > (3)
> > +   /*
> > +* Sanity check
> > +*/
> > +   if (!xact_started)
> > +   {
> > +   ereport(ERROR,
> > +
>   (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > +errmsg("Transaction
> must have been already started to set statement timeout")));
> > +   }
> >
> > I think this fragment can be deleted, because enable/disable_timeout()
> is used only at limited places in postgres.c, so I don't see the chance
> of misuse.
> 
> I'd suggest leave it as it is. Because it might be possible that the function
> is used in different place in the future. Or at least we should document
> the pre-condition as a comment.

OK, I favor documenting to reduce code, but I don't have a strong opinion.  I'd 
like to leave this to the committer.  One thing to note is that you can remove 
{ and } in the following code like other places.

+   if (!xact_started)
+   {
+   ereport(ERROR,
+   
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+errmsg("Transaction must have 
been already started to set statement timeout")));
+   }

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] wait event documentation

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
> Instead of continuing to repair this every time it gets broken, I propose
> that we break this into one table that lists all the wait_event_type values
> -- LWLock, Lock, BufferPin, Activity, Client, Extension, IPC, Timeout, and
> IO -- and then a second table for each type that has multiple wait events
> listing all of the wait events for that type.
> 
> I also propose hoisting this out of section 28.2.3 - Viewing Statistics
> - and making it a new toplevel section of chapter 28.  So between the current
> "28.3 Viewing Locks" and the current "28.4 Progress Reporting" we'd add
> a new section "Wait Events" and link to that from 28.2.3.  That would also
> give us a place to include any general text that we want to have regarding
> wait events, apart from the tables.

+1
I'm visually impaired and using screen reader software which reads text by 
Synthetic speech.  It was not easy for me to navigate a large table to find the 
first row of a particular wait event type.  With your proposal, I'll be able to 
move among tables with a single key press ("T" and "Shift + T".)  I'd also be 
happy about separating the section for wait events as you propose, because 
navigating in a long section is cumbersome.

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] Statement timeout behavior in extended queries

[Tsunakawa, Takayuki]

Hello,


I've reviewed the patch.  My understanding is as follows.  Please correct me if 
I'm wrong.

* The difference is that the Execute message stops the statement_timeout timer, 
so that the execution of one statement doesn't shorten the timeout period of 
subsequent statements when they are run in batch followed by a single Sync 
message.

* This patch is also necessary (or desirable) for the feature "Pipelining/batch 
mode support for libpq," which is being developed for PG 10.  This patch 
enables correct timeout handling for each statement execution in a batch.  
Without this patch, the entire batch of statements is subject to 
statement_timeout.  That's different from what the manual describes about 
statement_timeout.  statement_timeout should measure execution of each 
statement.


Below are what I found in the patch.


(1)
+static bool st_timeout = false;

I think the variable name would better be stmt_timeout_enabled or 
stmt_timer_started, because other existing names use stmt to abbreviate 
statement, and thhis variable represents a state (see xact_started for example.)


(2)
+static void enable_statement_timeout(void)
+{

+static void disable_statement_timeout(void)
+{

"static void" and the remaining line should be on different lines, as other 
functions do.



(3)
+   /*
+* Sanity check
+*/
+   if (!xact_started)
+   {
+   ereport(ERROR,
+   
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+errmsg("Transaction must have 
been already started to set statement timeout")));
+   }

I think this fragment can be deleted, because enable/disable_timeout() is used 
only at limited places in postgres.c, so I don't see the chance of misuse.


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] Supporting huge pages on Windows

[Tsunakawa, Takayuki]

From: Michael Paquier [mailto:michael.paqu...@gmail.com]
> Amit is right, you had better use %zu as we do in all the other places of
> the code dealing with Size variables that are printed. When compiling with
> Visual Studio, Postgres falls back to the implementation of sprintf in
> src/port/snprintf.c so that's not something to worry about.

Thanks, fixed.

Regards
Takayuki Tsunakawa



win_large_pages_v12.patch
Description: win_large_pages_v12.patch

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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Alvaro Herrera
> Ashutosh Bapat wrote:
> > Please add this to the next commitfest.
> 
> If this cannot be reproduced in 9.6, then it must be added to the Open Items
> wiki page instead.

I added this in next CF.

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] Supporting huge pages on Windows

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Amit Kapila
> You should use %zu instead of %llu to print Size type of variable.

I did so at first, but it didn't work with Visual Studio 2010 at hand.  It 
doesn't support %z which is added in C99.

But "I" (capital i) instead of "ll" seems appropriate as the following page 
says.  I chose this.

https://en.wikipedia.org/wiki/Printf_format_string

I For signed integer types, causes printf to expect ptrdiff_t-sized integer 
argument; for unsigned integer types, causes printf to expect size_t-sized 
integer argument. Commonly found in Win32/Win64 platforms. 

Regards
Takayuki Tsunakawa




win_large_pages_v11.patch
Description: win_large_pages_v11.patch

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

Re: [HACKERS] Allow interrupts on waiting standby

[Tsunakawa, Takayuki]

From: Michael Paquier [mailto:michael.paqu...@gmail.com]
> Oops, sorry for that, I quite mess up with this patch. The WaitLatch() call
> should still have WL_POSTMASTER_DEATH so as it can leave earlier, but yes
> I agree with your analysis that HandleStartupProcInterrupts() as this is
> part of the redo work.

Thank you, but did you remove WL_LATCH_SET from WaitLatch() intentionally?  I 
understood you added it for startup process to respond quickly to events other 
than the postmaster death.  Why don't we restore WL_LATCH_SET?  I won't object 
to not adding the flag if there's a reason.

I'll mark this as ready for committer when I see WL_LATCH_SET added (optional) 
and you have reported that you did the following test cases:

* Startup process vanishes immediately after postmaster dies, while it is 
waiting for a recovery conflict to be resolved.

* Startup process vanishes immediately after "pg_ctl stop -m fast", while it is 
waiting for a recovery conflict to be resolved.

* Startup process resumes WAL application when max_standby_{archive | 
streaming}_delay is changed from the default -1 to a short period, e.g. 10s, 
and "pg_ctl reload" is performed, while it is waiting for a recovery conflict 
to be resolved.


> > Did Simon's committed patch solve the problem as expected?
> 
> Does not seem so, I'll let Simon comment on this matter...

Agreed.  I guess his patch for earlier releases should work if 
CHECK_FOR_INTERRUPTS() is replaced with HandleStartupProcInterrupts().

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

[HACKERS] [bug fix] Savepoint-related statements terminates connection

[Tsunakawa, Takayuki]

Hello,

I found a trivial bug that terminates the connection.  The attached patch fixes 
this.


PROBLEM


Savepoint-related statements in a multi-command query terminates the connection 
unexpectedly, as follows.

$ psql -d postgres -c "SELECT 1; SAVEPOINT sp"
FATAL:  DefineSavepoint: unexpected state STARTED
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
connection to server was lost


CAUSE


1. In exec_simple_query(), isTopLevel is set to false.

isTopLevel = (list_length(parsetree_list) == 1);

Then it is passed to PortalRun().

(void) PortalRun(portal,
 FETCH_ALL,
 isTopLevel,
 receiver,
 receiver,
 completionTag);

2. The isTopLevel flag is passed through ProcessUtility() to 
RequireTransactionChain().


RequireTransactionChain(isTopLevel, "SAVEPOINT");


3. CheckTransactionChain() returns successfully here:

/*
 * inside a function call?
 */
if (!isTopLevel)
return;


4. Finally, unexpectedly called DefineSavepoint() reports invalid transaction 
block state.

/* These cases are invalid. */
case TBLOCK_DEFAULT:
case TBLOCK_STARTED:
...
elog(FATAL, "DefineSavepoint: unexpected state %s",
 BlockStateAsString(s->blockState));



SOLUTION


The manual page says "Savepoints can only be established when inside a 
transaction block."  So just check for it.

https://www.postgresql.org/docs/devel/static/sql-savepoint.html


RESULT AFTER FIX


$ psql -d postgres -c "SELECT 1; SAVEPOINT sp"
ERROR:  SAVEPOINT can only be used in transaction blocks


Regards
Takayuki Tsunakawa



savept-in-multi-cmd.patch
Description: savept-in-multi-cmd.patch

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

Re: [HACKERS] Supporting huge pages on Windows

[Tsunakawa, Takayuki]

From: Amit Kapila [mailto:amit.kapil...@gmail.com]
> The latest patch looks good to me apart from one Debug message, so I have
> marked it as Ready For Committer.

Thank you so much!


> + ereport(DEBUG1,
> + (errmsg("disabling huge pages")));
> 
> I think this should be similar to what we display in sysv_shmem.c as below:
> 
> elog(DEBUG1, "mmap(%zu) with MAP_HUGETLB failed, huge pages disabled: %m",
> allocsize);

I understood you suggested this to make the reason clear for disabling huge 
pages.  OK, done as follows.

+   elog(DEBUG1, "CreateFileMapping(%llu) with SEC_LARGE_PAGES 
failed "
+"due to insufficient large pages, huge pages disabled",
+size);

I hope this will be committed soon.


Regards
Takayuki Tsunakawa



win_large_pages_v10.patch
Description: win_large_pages_v10.patch

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

Re: [HACKERS] Allow interrupts on waiting standby

[Tsunakawa, Takayuki]

Hi Michael, Simon,

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> > Oh, I see.  But how does the startup process respond quickly?  It seems
> that you need to call HandleStartupProcInterrupts() instead of
> CHECK_FOR_INTERRUPTS().  But I'm not sure whether
> HandleStartupProcInterrupts() can be called here.
> 
> Bah. Of course you are right. We don't care about SetLatch() here as signals
> are processed with a different code path than normal backends.

So, I understood you agreed that CHECK_FOR_INTERRUPTS() here does nothing.  But 
your patch still calls it:

+   /* An interrupt may have occurred while waiting */
+   CHECK_FOR_INTERRUPTS();

I got confused because the problem is not defined in this thread.  What problem 
does this patch address?  These ones?

* The startup process terminates as soon as postmaster dies.
* pg_stat_activity does not show the wait event of startup process waiting for 
a recovery conflict resolution.


My guess about why you decided to not call HandleStartupProcInterrupts() here 
is:

* Respond to postmaster death here.
* No need to reload config file here because there's no parameter to affect 
this conflict wait.  But max_standby_{archive | streaming}_delay seems to 
affect the wait period.
* No need to handle SIGTERM and exit here, because the startup process doesn't 
wait for a conflict resolution here when he can terminate.

I think you can call HandleStartupProcInterrupts() here, instead of checking 
postmaster death.  Did you perform tests?

Did Simon's committed patch solve the problem as expected?

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] Allow interrupts on waiting standby

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> > By the way, doesn't this wait event belong to IPC wait event type, because
> the process is waiting for other conflicting processes to terminate the
> conflict conditions?  Did you choose Timeout type because
> max_standby_{archive | streaming}_delay relates to this wait?  I'm not
> confident which is appropriate, but I'm afraid users can associate this
> wait with a timeout.
> 
> Actually I think that this event belongs to the timeout category, because
> we wait until the timeout has finished, the latch being here to be sure
> that the process is more responsive in case of a postmaster death.

OK.  I confirmed the doc is fixed.

> > (2) standby.c
> > Do we need to specify WL_LATCH_SET?  Who can set the latch?  Do the
> backends who ended the conflict set the latch?
> 
> This makes the process able to react on SIGHUP. That's useful for
> responsiveness.

Oh, I see.  But how does the startup process respond quickly?  It seems that 
you need to call HandleStartupProcInterrupts() instead of 
CHECK_FOR_INTERRUPTS().  But I'm not sure whether HandleStartupProcInterrupts() 
can be called here.



> > (3) standby.c
> > +   if (rc & WL_LATCH_SET)
> > +   ResetLatch(MyLatch);
> > +
> > +   /* emergency bailout if postmaster has died */
> > +   if (rc & WL_POSTMASTER_DEATH)
> > +   proc_exit(1);
> >
> > I thought the child processes have to terminate as soon as postmaster
> vanishes.  So, it would be better for the order of the two if statements
> above to be reversed.  proc_exit() could be exit(), as some children do
> in postmaster/*.c.
> 
> OK, reversed this order.

I think exit() instead of proc_exit() better represents what the code wants to 
do -- terminate the process ASAP without cleaning up.  Many other background 
children do so.


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] Allow interrupts on waiting standby

[Tsunakawa, Takayuki]

 (4) standby.c
> The latch is not reset when the wait timed out.  The next WaitLatch() would
> return immediately.

Sorry, let me withdraw this.  This is my misunderstanding.

OTOH, when is the latch reset before the wait?  Is there an assumption that 
MyLatch has been in reset state since it was initialized?
Is it safe to use MyLatch here, not the special latch like something used in 
xlog.c?


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] Allow interrupts on waiting standby

[Tsunakawa, Takayuki]

Hi, Michael,


From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> What do you think about the updated version attached?

I reviewed this patch.  Here are some comments and questions:


(1) monitoring.sgml
The new row needs to be placed in the "Timeout" group.  The current patch puts 
the row at the end of IO group.  Please insert the new row according to the 
alphabetical order.

In addition, "morerows" attribute of the following line appears to be increased.

 Timeout

By the way, doesn't this wait event belong to IPC wait event type, because the 
process is waiting for other conflicting processes to terminate the conflict 
conditions?  Did you choose Timeout type because max_standby_{archive | 
streaming}_delay relates to this wait?  I'm not confident which is appropriate, 
but I'm afraid users can associate this wait with a timeout.


(2) standby.c
Do we need to specify WL_LATCH_SET?  Who can set the latch?  Do the backends 
who ended the conflict set the latch?


(3) standby.c
+   if (rc & WL_LATCH_SET)
+   ResetLatch(MyLatch);
+
+   /* emergency bailout if postmaster has died */
+   if (rc & WL_POSTMASTER_DEATH)
+   proc_exit(1);

I thought the child processes have to terminate as soon as postmaster vanishes. 
 So, it would be better for the order of the two if statements above to be 
reversed.  proc_exit() could be exit(), as some children do in postmaster/*.c.



(4) standby.c
The latch is not reset when the wait timed out.  The next WaitLatch() would 
return immediately.


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] Supporting huge pages on Windows

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of David Steele
> It's not clear to me what state this patch should be in.  Is there more
> review that needs to be done or is it ready for a committer?

I would most appreciate it if Magnus could review the code, because he gave me 
the most critical comment that the value of huge_page variable should not be 
changed on the fly, and I did so.  I hope that will be the final review.

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] Crash on promotion when recovery.conf is renamed

[Tsunakawa, Takayuki]

From: Michael Paquier [mailto:michael.paqu...@gmail.com]
> Okay. I got the message, and I agree with what you say here. You are right
> by the way, the error messages just use "two-phase file" and not "two-phase
> STATE file", missed that previously.
> --
Thanks, marked as ready for committer, as the code is good and Alexander 
reported the test success.

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] Crash on promotion when recovery.conf is renamed

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> While I agree with that in general, we are taking about 2PC files that are
> on disk in patch 0001, so I would think that in this case
> ERRCODE_DATA_CORRUPTED is the most adapted (or
> ERRCODE_TWOPHASE_CORRUPTED?).
> 
> The other WARNING messages refer to stale files of already committed
> transactions, which are not actually corrupted. What about in this case
> having a ERRCODE_TWOPHASE_INVALID?
> 
> Updated patches are attached, I did not change the WARNING portion though
> as I am not sure what's the consensus on the matter.
> 

I get the impression that DATA_CORRUPTED means the table data is corrupted, 
because there's an error code named INDEX_CORRUPTED.  Anyway, I don't think 
this patch needs to attach an error code because:

* Currently, other interlal files (not tables or indexes) seem to use 
INTERNAL_ERROR (XX000).  For example, see ReadControlFile() in xlog.c and 
pgstat_read_statsfiles() in pgstat.c.

* It doesn't seem that the user needs distinction.  I don't object to providing 
a specific error code for this case, but if the patch needs a specific error 
code to be committed, I'd like to know how that's useful (e.g. how it affects 
the recovery action the user takes.)


So, I'd like to mark the patch as ready for committer when ereport() and 
errmsg() are on separate lines and the message changes to "two-phase state 
file."

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] On How To Shorten the Steep Learning Curve Towards PG Hacking...

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Kang Yuzhe

> 1. Prepare Hands-on with PG internals
> 
> 
>  For example, a complete Hands-on  with SELECT/INSERT SQL Standard PG
> internals. The point is the experts can pick one fairly complex feature
> and walk it from Parser to Executor in a hands-on manner explaining step
> by step every technical detail.

I sympathize with you.  What level of detail do you have in mind?  The query 
processing framework is described in the manual:

Chapter 50. Overview of PostgreSQL Internals
https://www.postgresql.org/docs/devel/static/overview.html

More detailed source code analysis is provided for very old PostgreSQL 7.4, but 
I guess it's not much different now.  The document is in Japanese only: 

http://ikubo.x0.com/PostgreSQL/pg_source.htm

Are you thinking of something like this?

MySQL Internals Manual
https://dev.mysql.com/doc/internals/en/





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] Crash on promotion when recovery.conf is renamed

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tom Lane
> "Tsunakawa, Takayuki" <tsunakawa.ta...@jp.fujitsu.com> writes:
> > All other places in twophase.c and most places in other files put ereport()
> and errmsg() on separate lines.  I think it would be better to align with
> surrounding code.
> 
> > +   ereport(FATAL, (errmsg("corrupted
> two-phase file \"%s\"",
> 
> Actually, the *real* problem with that coding is it lacks a SQLSTATE (errcode
> call).  The only places where it's acceptable to leave that out are for
> internal "can't happen" cases, which this surely isn't.

Oh, I overlooked it.  But...


> Yup.  Just remember that the default is
> XX000EERRCODE_INTERNAL_ERROR  internal_error
> 
> If that's not how you want the error case reported, you need an errcode()
> call.
> 
> We might need more ERRCODEs than are there now, if none of the existing
> ones seem to fit these cases.  There's already ERRCODE_DATA_CORRUPTED and
> ERRCODE_INDEX_CORRUPTED; maybe we need ERRCODE_WAL_CORRUPTED, for
> example?

I'd be always happy if the error code is more specific, but maybe that would be 
a separate patch.  WAL corruption message so far doesn't accompany a specific 
error code like this in xlog.c:

/*
 * We only end up here without a message when 
XLogPageRead()
 * failed - in that case we already logged something. In
 * StandbyMode that only happens if we have been 
triggered, so we
 * shouldn't loop anymore in that case.
 */
if (errormsg)
ereport(emode_for_corrupt_record(emode,

 RecPtr ? RecPtr : EndRecPtr),
(errmsg_internal("%s", errormsg) /* already 
translated */ ));

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] Potential data loss of 2PC files

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> Do you think that this qualifies as a bug fix for a backpatch? I would think
> so, but I would not mind waiting for some dust to be on it before considering
> applying that on back-branches. Thoughts from others?

I think so, too.  As change from rename() to durable_rename() was applied to 
all releases, maybe we can follow that.  In addition, keeping code differences 
as few as possible would make it easier for committers to apply other bug fixes 
to all versions.

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] Crash on promotion when recovery.conf is renamed

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> Moved to CF 2017-03. Both patches still apply.

Sorry to be late for reviewing this, but done now.  The patch applied, make 
check passed, and the code looks almost good.  I could successfully perform a 
simple archive recovery.  Finally, I broke the 2pc state file while the server 
is down, and could confirm that the server failed to start as expected, 
emitting a FATAL message.  Worked nicely.

Just two cosmetic points:

(1)
Other places use "two-phase state file", not "two-phase file".


(2)
All other places in twophase.c and most places in other files put ereport() and 
errmsg() on separate lines.  I think it would be better to align with 
surrounding code.

+   ereport(FATAL, (errmsg("corrupted two-phase 
file \"%s\"",


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] Do we create a new roadmap page for development?

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
> On Tue, Mar 21, 2017 at 2:36 AM, Tsunakawa, Takayuki
> <tsunakawa.ta...@jp.fujitsu.com> wrote:
> > Should I create a page for PostgreSQL 11 likewise?  Or, do you want a
> more stable page named "PostgreSQL Roadmap" instead of "PostgreSQL11
> Roadmap" and attach the target version to each feature as in "Feature X
> (V11)", so that we can represent more longer-term goals?
> 
> I think a page that's just called PostgreSQL Roadmap will get out of date
> pretty quickly.  Ultimately it'll end up like the TODO list, which is not
> really a list of things TO DO.  The advantage of the version-specific pages
> is that old information kind of ages its way out of the system.

Maybe so.  I created a page for PostgreSQL 11 and add a link to our roadmap:

https://wiki.postgresql.org/wiki/PostgreSQL11_Roadmap

Please add your roadmaps when you can.


> > And, why don't we add a link to the above roadmap page in the "Development
> information" page?
> >
> > Development information
> > https://wiki.postgresql.org/wiki/Development_information
> 
> I'm sure nobody will object to you doing that.  It's a wiki, and intended
> to be edited.

Thanks, done. Also, I moved the existing the link to "Development projects" 
from " Past PgCon Developer Meeting Notes" to the new header "Roadmaps and 
Projects", because the development projects page doesn't appear to fit the 
PGCON notes.

Regards
Takayuki


-- 
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 create a new roadmap page for development?

[Tsunakawa, Takayuki]

Hello,

I'd like to share our roadmap for PostgreSQL development, as other companies 
and individuals do in the following page.  But this page is for PostgreSQL 10.

PostgreSQL10 Roadmap
https://wiki.postgresql.org/wiki/PostgreSQL10_Roadmap

Should I create a page for PostgreSQL 11 likewise?  Or, do you want a more 
stable page named "PostgreSQL Roadmap" instead of "PostgreSQL11 Roadmap" and 
attach the target version to each feature as in "Feature X (V11)", so that we 
can represent more longer-term goals?

And, why don't we add a link to the above roadmap page in the "Development 
information" page?

Development information
https://wiki.postgresql.org/wiki/Development_information


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] PATCH: Make pg_stop_backup() archive wait optional

[Tsunakawa, Takayuki]

From: David Steele [mailto:da...@pgmasters.net]
> Well, that's embarrassing.  When I recreated the function to add defaults
> I messed up the AS clause and did not pay attention to the results of the
> regression tests, apparently.
> 
> Attached is a new version rebased on 88e66d1.  Catalog version bump has
> also been omitted.

I was late to reply because yesterday was a national holiday in Japan.  I 
marked this as ready for committer.  The patch applied cleanly and worked as 
expected.

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] PATCH: Make pg_stop_backup() archive wait optional

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tsunakawa,
> Takayuki
> I made this ready for committer.  The patch applied except for catversion.h,
> the patch content looks good, and the target test passed as follows:

Sorry, I reverted this to waiting for author, because make check failed.  
src/test/regress/expected/opr_sanity.out seems to need revision because 
pg_stop_backup() was added.

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] Potential data loss of 2PC files

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Ashutosh Bapat
> The scope of this work has expanded, since last time I reviewed and marked
> it as RFC. Right now I am busy with partition-wise joins and do not have
> sufficient time to take a look at the expanded scope.
> However, I can come back to this after partition-wise join, but that may
> stretch to the end of the commitfest. Sorry.

I marked this as ready for committer.  The code looks good, make check passed, 
ran read/write pgbench for some period to cause checkpoint with WAL file 
removal, and pg_ctl stop.  The checkpoint emitted the following message, and 
there were no message like "could not ..." that indicates a file unlink or 
directory sync failure.

LOG:  checkpoint complete: wrote 1835 buffers (11.2%); 0 transaction log 
file(s) added, 1 removed, 0 recycled; write=0.725 s, sync=0.002 s, total=0.746 
s; s\
ync files=9, longest=0.002 s, average=0.000 s; distance=16381 kB, 
estimate=16381 kB

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