Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

[Yugo Nagata]

On Wed, 29 Aug 2018 21:09:03 +0900
Yugo Nagata  wrote:

> On Wed, 29 Aug 2018 13:46:38 +0200
> Magnus Hagander  wrote:
> 
> > On Wed, Aug 29, 2018 at 1:37 PM, Michael Banck 
> > wrote:
> > 
> > > Hi,
> > >
> > > On Wed, Aug 29, 2018 at 08:33:43PM +0900, Yugo Nagata wrote:
> > > > On Wed, 29 Aug 2018 10:28:33 +0200
> > > > Daniel Gustafsson  wrote:
> > > > > > On 27 Aug 2018, at 14:05, Yugo Nagata  wrote:
> > > > > > On Mon, 27 Aug 2018 13:34:12 +0200
> > > > > > Michael Banck  wrote:
> > > > > >> On Mon, Aug 27, 2018 at 07:53:36PM +0900, Yugo Nagata wrote:
> > > > > >>> On Fri, 24 Aug 2018 18:01:09 +0200
> > > > > >>> Peter Eisentraut  wrote:
> > > > > >>>> I'm curious about this option:
> > > > > >>>>
> > > > > >>>>  -r RELFILENODE check only relation with specified
> > > relfilenode
> > > > > >>>>
> > > > > >>>> but there is no facility to specify a database.
> > > > > >>>>
> > > > > >>>> Also, referring to the relfilenode of a mapped relation seems a
> > > bit
> > > > > >>>> inaccurate.
> > > > > >>>>
> > > > > >>>> Maybe reframing this in terms of the file name of the file you
> > > want
> > > > > >>>> checked would be better?
> > > > > >>>
> > > > > >>> If we specified 1234 to -r option, pg_verify_shceksums checks not
> > > only 1234
> > > > > >>> but also 1234_vm, 1234_fsm, and 1234.1, 1234.2, ... and so on, so
> > > I think
> > > > > >>> it makes senses to allow to specify a relfilenode instead of a
> > > file name.
> > > > > >>>
> > > > > >>> I think it is reasonable to add a option to specify a database,
> > > although
> > > > > >>> I don't know which character is good because both -d and -D are
> > > already used
> > > > > >>
> > > > > >> Maybe the -d (debug) option should be revisited as well. Mentioning
> > > > > >> every scanned block generates a huge amount of output which might 
> > > > > >> be
> > > > > >> useful during development but does not seem very useful for a 
> > > > > >> stable
> > > > > >> release. AFAICT there is no other debug output for now.
> > > > > >>
> > > > > >> So it could be renamed to -v (verbose) and only mention each 
> > > > > >> scanned
> > > > > >> file, e.g. (errors/checksum mismatches are still reported of
> > > course).
> > >
> > > I still think this should be changed as well, i.e. -v should not report
> > > every block scanned, as that really is debug output and IMO not useful
> > > in general? AFAICT your page does not change the output at all, just
> > > renames the option.
> > >
> > >
> > I agree with this (though it's my fault initially :P). Per-page output is
> > going to be useless in pretty much all production cases. It makes sense to
> > also change it to be per-file.
> 
> I updated the patch to output only per-file information in the verbose mode.
> Does this behavior match you expect?

I am sorry but I attached a wrong file in the previous post.
Attached is the correct version of the updated patch.

Regards,
-- 
Yugo Nagata 
commit 7c673e1d712c74c51c708ddc4a151b6fb8cc2a8f
Author: Yugo Nagata 
Date:   Wed Aug 29 19:37:13 2018 +0900

Raname pg_verity_checksums debug option -d to -v / verbose

Also, change to only mention each scanced file not every block.

diff --git a/doc/src/sgml/ref/pg_verify_checksums.sgml b/doc/src/sgml/ref/pg_verify_checksums.sgml
index ecc5501eae..a1ff060c2b 100644
--- a/doc/src/sgml/ref/pg_verify_checksums.sgml
+++ b/doc/src/sgml/ref/pg_verify_checksums.sgml
@@ -64,8 +64,8 @@ PostgreSQL documentation
   -d
   

-Enable debug output. Lists all checked blocks and their checksum.
-   
+Enable debug output. Lists all checked files.
+   
   
  
 
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 938b92282a..6be138eb75 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums

Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

[Yugo Nagata]

On Wed, 29 Aug 2018 13:46:38 +0200
Magnus Hagander  wrote:

> On Wed, Aug 29, 2018 at 1:37 PM, Michael Banck 
> wrote:
> 
> > Hi,
> >
> > On Wed, Aug 29, 2018 at 08:33:43PM +0900, Yugo Nagata wrote:
> > > On Wed, 29 Aug 2018 10:28:33 +0200
> > > Daniel Gustafsson  wrote:
> > > > > On 27 Aug 2018, at 14:05, Yugo Nagata  wrote:
> > > > > On Mon, 27 Aug 2018 13:34:12 +0200
> > > > > Michael Banck  wrote:
> > > > >> On Mon, Aug 27, 2018 at 07:53:36PM +0900, Yugo Nagata wrote:
> > > > >>> On Fri, 24 Aug 2018 18:01:09 +0200
> > > > >>> Peter Eisentraut  wrote:
> > > > >>>> I'm curious about this option:
> > > > >>>>
> > > > >>>>  -r RELFILENODE check only relation with specified
> > relfilenode
> > > > >>>>
> > > > >>>> but there is no facility to specify a database.
> > > > >>>>
> > > > >>>> Also, referring to the relfilenode of a mapped relation seems a
> > bit
> > > > >>>> inaccurate.
> > > > >>>>
> > > > >>>> Maybe reframing this in terms of the file name of the file you
> > want
> > > > >>>> checked would be better?
> > > > >>>
> > > > >>> If we specified 1234 to -r option, pg_verify_shceksums checks not
> > only 1234
> > > > >>> but also 1234_vm, 1234_fsm, and 1234.1, 1234.2, ... and so on, so
> > I think
> > > > >>> it makes senses to allow to specify a relfilenode instead of a
> > file name.
> > > > >>>
> > > > >>> I think it is reasonable to add a option to specify a database,
> > although
> > > > >>> I don't know which character is good because both -d and -D are
> > already used
> > > > >>
> > > > >> Maybe the -d (debug) option should be revisited as well. Mentioning
> > > > >> every scanned block generates a huge amount of output which might be
> > > > >> useful during development but does not seem very useful for a stable
> > > > >> release. AFAICT there is no other debug output for now.
> > > > >>
> > > > >> So it could be renamed to -v (verbose) and only mention each scanned
> > > > >> file, e.g. (errors/checksum mismatches are still reported of
> > course).
> >
> > I still think this should be changed as well, i.e. -v should not report
> > every block scanned, as that really is debug output and IMO not useful
> > in general? AFAICT your page does not change the output at all, just
> > renames the option.
> >
> >
> I agree with this (though it's my fault initially :P). Per-page output is
> going to be useless in pretty much all production cases. It makes sense to
> also change it to be per-file.

I updated the patch to output only per-file information in the verbose mode.
Does this behavior match you expect?

Regards,
-- 
Yugo Nagata 
commit ec21c608cd78f65747916487b56a197c685649c8
Author: Yugo Nagata 
Date:   Wed Aug 29 19:37:13 2018 +0900

Raname pg_verity_checksums debug option -d to -v / verbose

Also, change to only mention each scanced file not every block.

diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 938b92282a..0f931b7579 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -31,7 +31,7 @@ static int64 badblocks = 0;
 static ControlFileData *ControlFile;
 
 static char *only_relfilenode = NULL;
-static bool debug = false;
+static bool verbose = false;
 
 static const char *progname;
 
@@ -43,7 +43,7 @@ usage()
 	printf(_("  %s [OPTION]... [DATADIR]\n"), progname);
 	printf(_("\nOptions:\n"));
 	printf(_(" [-D, --pgdata=]DATADIR  data directory\n"));
-	printf(_("  -d debug output, list all checked blocks\n"));
+	printf(_("  -v, --verbose  output verbose messages, list all checked blocks\n"));
 	printf(_("  -r RELFILENODE check only relation with specified relfilenode\n"));
 	printf(_("  -V, --version  output version information, then exit\n"));
 	printf(_("  -?, --help show this help, then exit\n"));
@@ -120,7 +120,7 @@ scan_file(char *fn, int segmentno)
 		progname, fn, blockno, csum, header->pd_checksum);
 			badblocks++;
 		}
-		else if (debug)
+		else if (verbose)
 			fprintf(stderr, _("%s: checksum verified in file \"%s\", block %d: %X\n"),
 	progname, fn, blockno, csum);
 	}
@@ -208,6 +208,7 @@ main(int argc, char *argv[])
 {
 	static struct option long_options[] = {
 		{"pgdata", required_argument, NULL, 'D'},
+		{"verbose", no_argument, NULL, 'v'},
 		{NULL, 0, NULL, 0}
 	};
 
@@ -234,12 +235,12 @@ main(int argc, char *argv[])
 		}
 	}
 
-	while ((c = getopt_long(argc, argv, "D:r:d", long_options, _index)) != -1)
+	while ((c = getopt_long(argc, argv, "D:r:v", long_options, _index)) != -1)
 	{
 		switch (c)
 		{
-			case 'd':
-debug = true;
+			case 'v':
+verbose = true;
 break;
 			case 'D':
 DataDir = optarg;

Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

[Yugo Nagata]

On Wed, 29 Aug 2018 10:28:33 +0200
Daniel Gustafsson  wrote:

> > On 27 Aug 2018, at 14:05, Yugo Nagata  wrote:
> > 
> > On Mon, 27 Aug 2018 13:34:12 +0200
> > Michael Banck  wrote:
> > 
> >> Hi,
> >> 
> >> On Mon, Aug 27, 2018 at 07:53:36PM +0900, Yugo Nagata wrote:
> >>> On Fri, 24 Aug 2018 18:01:09 +0200
> >>> Peter Eisentraut  wrote:
> >>>> I'm curious about this option:
> >>>> 
> >>>>  -r RELFILENODE check only relation with specified relfilenode
> >>>> 
> >>>> but there is no facility to specify a database.
> >>>> 
> >>>> Also, referring to the relfilenode of a mapped relation seems a bit
> >>>> inaccurate.
> >>>> 
> >>>> Maybe reframing this in terms of the file name of the file you want
> >>>> checked would be better?
> >>> 
> >>> If we specified 1234 to -r option, pg_verify_shceksums checks not only 
> >>> 1234
> >>> but also 1234_vm, 1234_fsm, and 1234.1, 1234.2, ... and so on, so I think
> >>> it makes senses to allow to specify a relfilenode instead of a file name.
> >>> 
> >>> I think it is reasonable to add a option to specify a database, although
> >>> I don't know which character is good because both -d and -D are already 
> >>> used
> >> 
> >> Maybe the -d (debug) option should be revisited as well. Mentioning
> >> every scanned block generates a huge amount of output which might be
> >> useful during development but does not seem very useful for a stable
> >> release. AFAICT there is no other debug output for now.
> >> 
> >> So it could be renamed to -v (verbose) and only mention each scanned
> >> file, e.g. (errors/checksum mismatches are still reported of course).
> >> 
> >> Then -d could (in the future, I guess that is too late for v11) be used
> >> for -d/--dbname (or make that only a long option, if the above does not
> >> work).
> > 
> > I realized after sending the previous post that we can not specify a 
> > database
> > by name because pg_verify_checksum is run in offline and this can not get 
> > the
> > OID from the database name.  Also, there are global and pg_tblspc 
> > directories
> > not only base/. So, it seems to me good to specify a 
> > directories
> > to scan which is under PGDATA. We would be able to use -d ( or --directory 
> > ?)
> > for this purpose.
> 
> Changing functionality to the above discussed is obviously 12 material, but
> since we are discussing changing the command line API of the tool by
> repurposing -d; do we want to rename the current use of -d to -v (with the
> accompanying ―-verbose) before 11 ships?  It’s clearly way way too late in the
> cycle but it seems worth to at least bring up since 11 will be the first
> version pg_verify_checksums ship in. I’m happy to do the work asap if so.

I agree with this.  Almost other commands doesn't use -d as debug mode
although there a few exception, and instead -v is used for verbose mode.
If we can change the command line of pg_veriry_checksums, before release of
PG 11 is best.  Attached is the patch to do this.

Regards,
-- 
Yugo Nagata 
commit 452f981c30c9d9c7263e6006dc4cda51278ff376
Author: Yugo Nagata 
Date:   Wed Aug 29 19:37:13 2018 +0900

Raname pg_verity_checksums debug option -d to -v / verbose

diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 938b92282a..0f931b7579 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -31,7 +31,7 @@ static int64 badblocks = 0;
 static ControlFileData *ControlFile;
 
 static char *only_relfilenode = NULL;
-static bool debug = false;
+static bool verbose = false;
 
 static const char *progname;
 
@@ -43,7 +43,7 @@ usage()
 	printf(_("  %s [OPTION]... [DATADIR]\n"), progname);
 	printf(_("\nOptions:\n"));
 	printf(_(" [-D, --pgdata=]DATADIR  data directory\n"));
-	printf(_("  -d debug output, list all checked blocks\n"));
+	printf(_("  -v, --verbose  output verbose messages, list all checked blocks\n"));
 	printf(_("  -r RELFILENODE check only relation with specified relfilenode\n"));
 	printf(_("  -V, --version  output version information, then exit\n"));
 	printf(_("  -?, --help show this help, then exit\n"));
@@ -120,7 +120,7 @@ scan_file(char *fn, int segmentno)
 		progname, fn, blockno, csum, header->pd_checksum);
 

Re: pg_verify_checksums failure with hash indexes

[Yugo Nagata]

On Wed, 29 Aug 2018 16:01:53 +0530
Amit Kapila  wrote:

> > By the way, I think we can fix this also by clearing the header information 
> > of the last
> > page instead of setting a checksum to the unused page although I am not 
> > sure which way
> > is better.
> >
> 
> I think that can complicate the WAL logging of this operation which we
> are able to deal easily with log_newpage and it sounds quite hacky.
> The fix I have posted seems better, but I am open to suggestions.

Thank you for your explanation.  I understood  this way could make the
codes complicated, so I think the way you posted is better.


Regards,
-- 
Yugo Nagata 

Re: pg_verify_checksums failure with hash indexes

[Yugo Nagata]

On Wed, 29 Aug 2018 14:39:10 +0530
Amit Kapila  wrote:

> On Tue, Aug 28, 2018 at 2:51 PM Peter Eisentraut
>  wrote:
> >
> > This is reproducible with PG11 and PG12:
> >
> > initdb -k data
> > postgres -D data
> >
> > make installcheck
> > # shut down postgres with Ctrl-C
> >
> ..
> >
> > The files in question correspond to
> >
> > hash_i4_index
> > hash_name_index
> > hash_txt_index
> >
> 
> I have looked into this problem and found the cause of it.  This
> problem is happening for the empty page in the hash index.  On a
> split, we allocate a new splitpoint's worth of bucket pages wherein we
> initialize the last page with zero's, this is all fine, but we forgot
> to set the checksum for that last page.  Attached patch fixes the
> problem for me.
> 
> Can someone try and share their findings?

I confirmed that this patch fixed the problem by setting a checksum in the last
page in hash indexes, and pg_veviry_checksum is done successfully.  

regression=# select * from page_header(get_raw_page('hash_i4_index',65));
lsn| checksum | flags | lower | upper | special | pagesize | version | 
prune_xid 
---+--+---+---+---+-+--+-+---
 0/41CACF0 |18720 | 0 |24 |  8176 |8176 | 8192 |   4 |  
   0
(1 row)

By the way, I think we can fix this also by clearing the header information of 
the last
page instead of setting a checksum to the unused page although I am not sure 
which way
is better.

Regards,

-- 
Yugo Nagata 

Re: pg_verify_checksums failure with hash indexes

[Yugo Nagata]

valid special section encountered.
>  Error: Special section points off page. Unable to dump contents.
> 
> Block   65 
>  -
>  Block Offset: 0x00082000 Offsets: Lower  24 (0x0018)
>  Block: Size 8192  Version4Upper8176 (0x1ff0)
>  LSN:  logid  0 recoff 0x04229c20  Special  8176 (0x1ff0)
>  Items:0  Free Space: 8152
>  Checksum: 0x  Prune XID: 0x  Flags: 0x ()
>  Length (including item array): 24
> 
>  Error: checksum failure: calculated 0x4692.
> 
>   :  209c2204  1800f01f   .".
>   0010: f01f0420 ... 
> 
>  -- 
>  Empty block - no items listed 
> 
>  -
>  Hash Index Section:
>   Flags: 0x ()
>   Bucket Number: 0x
>   Blocks: Previous (-1)  Next (-1)
> 
>   1ff0:    80ff  
> 
> 
> *** End of Requested Range Encountered. Last Block Read: 65 ***
> --8<--
> 
> So it seems there is some data on the last page, which makes the zero
> checksum bogus, but I don't know anything about hash indexes. Also maybe
> those empty pages are not initialized correctly? Or maybe the "Invalid
> special section encountered" error meand pg_filedump cannot handle hash
> indexes completely.

I saw the same thing in the hash_i4_index case using pageinspect with
checksum disablbed. The last page (block 65) has some data in its header. 

regression=# select * from page_header(get_raw_page('hash_i4_index',65));
lsn| checksum | flags | lower | upper | special | pagesize | version | 
prune_xid 
---+--+---+---+---+-+--+-+---
 0/939FE48 |0 | 0 |24 |  8176 |8176 | 8192 |   4 |  
   0
(1 row)

Looking at the code to check the checksum, each page is checked if this is a
new page by using PageIsNew(), and if so its checksum is not checked because
new pages are assumed to have no checksum.  PageIsNew() determines if a
page is new or not from pd_upper.  For some reason, the last page has pd_upper
but no checksum, so the checksum verification fails.

It is not clear for me why the last page has a head information, but, after
some code investigation, I think it happend in _hash_alloc_buckets().  When
expanding a hash table, smgrextend() add some blocks to a file. At that time,
it seems that a page that has a header infomation is written to the end of
the file (in mdextend()).

I'm not sure how to fix this for now, but it might be worth to share my 
analysis for this issue.

Regards,
-- 
Yugo Nagata 

Re: Refactor textToQualifiedNameList()

[Yugo Nagata]

On Tue, 28 Aug 2018 11:49:26 +0900 (Tokyo Standard Time)
Kyotaro HORIGUCHI  wrote:

> At Fri, 24 Aug 2018 20:44:12 +0900, Yugo Nagata  wrote 
> in <20180824204412.150979ae6b283ddb639f9...@sraoss.co.jp>
> > When working on other patch[1], I found there are almost same
> > functions, texttoQualifiedNameList() and stringToQualifiedNameList().
> > The only difference is the argument type, text or char*. I don't know
> > why these functions are defined seperately, but I think the former 
> > function can be rewritten using the latter code as the attached patch.
> > Is this reasonable fix?
> 
> The functions were introduced within a month for different
> objectives in March and April, 2002. I supppose that they are
> intentionally added as separate functions for simplicitly since
> the second one is apparent CnP'ed from the first one.
> 
> commit 5f4745adf4fb2a1f933b25d7a2bc72b39fa9edfd
> commit 52200befd04b9fa71da83231c808764867079226

Thank you for your comments. I also looked at the commit history.
stringToQNL is added after textToQNL as a static function originally,
but this is now public and reffered from other modules, too.  So, I 
propose to mote this from regproc.c to verlena.c and rewrite textToQNL
to call stringToQNL.  This is just for reducing redundancy of the code. 
Attached is the updated patch.
 
> Returning to the patch, the downside of it is that textToQNL
> makes an extra and unused copy of the parameter string. (It's a
> kind of bug that it is forgetting to free rawname.)

I also fixed to free rawname in the texttoQNL.

Regards,

-- 
Yugo Nagata 
diff --git a/src/backend/utils/adt/regproc.c b/src/backend/utils/adt/regproc.c
index a0079821fe..ebc087ed3f 100644
--- a/src/backend/utils/adt/regproc.c
+++ b/src/backend/utils/adt/regproc.c
@@ -1680,43 +1680,6 @@ text_regclass(PG_FUNCTION_ARGS)
 }
 
 
-/*
- * Given a C string, parse it into a qualified-name list.
- */
-List *
-stringToQualifiedNameList(const char *string)
-{
-	char	   *rawname;
-	List	   *result = NIL;
-	List	   *namelist;
-	ListCell   *l;
-
-	/* We need a modifiable copy of the input string. */
-	rawname = pstrdup(string);
-
-	if (!SplitIdentifierString(rawname, '.', ))
-		ereport(ERROR,
-(errcode(ERRCODE_INVALID_NAME),
- errmsg("invalid name syntax")));
-
-	if (namelist == NIL)
-		ereport(ERROR,
-(errcode(ERRCODE_INVALID_NAME),
- errmsg("invalid name syntax")));
-
-	foreach(l, namelist)
-	{
-		char	   *curname = (char *) lfirst(l);
-
-		result = lappend(result, makeString(pstrdup(curname)));
-	}
-
-	pfree(rawname);
-	list_free(namelist);
-
-	return result;
-}
-
 /*
  *	 SUPPORT ROUTINES		 *
  */
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index a5e812d026..1846ddcae8 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -3219,26 +3219,19 @@ name_text(PG_FUNCTION_ARGS)
 	PG_RETURN_TEXT_P(cstring_to_text(NameStr(*s)));
 }
 
-
 /*
- * textToQualifiedNameList - convert a text object to list of names
- *
- * This implements the input parsing needed by nextval() and other
- * functions that take a text parameter representing a qualified name.
- * We split the name at dots, downcase if not double-quoted, and
- * truncate names if they're too long.
+ * Given a C string, parse it into a qualified-name list.
  */
 List *
-textToQualifiedNameList(text *textval)
+stringToQualifiedNameList(const char *string)
 {
 	char	   *rawname;
 	List	   *result = NIL;
 	List	   *namelist;
 	ListCell   *l;
 
-	/* Convert to C string (handles possible detoasting). */
-	/* Note we rely on being able to modify rawname below. */
-	rawname = text_to_cstring(textval);
+	/* We need a modifiable copy of the input string. */
+	rawname = pstrdup(string);
 
 	if (!SplitIdentifierString(rawname, '.', ))
 		ereport(ERROR,
@@ -3263,6 +3256,31 @@ textToQualifiedNameList(text *textval)
 	return result;
 }
 
+
+/*
+ * textToQualifiedNameList - convert a text object to list of names
+ *
+ * This implements the input parsing needed by nextval() and other
+ * functions that take a text parameter representing a qualified name.
+ * We split the name at dots, downcase if not double-quoted, and
+ * truncate names if they're too long.
+ */
+List *
+textToQualifiedNameList(text *textval)
+{
+	char	   *rawname;
+	List	   *result = NIL;
+
+	/* Convert to C string (handles possible detoasting). */
+	/* Note we rely on being able to modify rawname below. */
+	rawname = text_to_cstring(textval);
+
+	result = stringToQualifiedNameList(rawname);
+	pfree(rawname);
+
+	return result;
+}
+
 /*
  * SplitIdentifierString --- parse a string containing identifiers
  *
diff --git a/src/include/utils/regproc.h b/src/include/utils/regproc.h
index 5b9a8

Re: libpq debug log

[Yugo Nagata]

On Fri, 24 Aug 2018 04:38:22 +
"Iwata, Aya"  wrote:

> Hi,
> 
> I'm going to propose libpq debug log for analysis of queries on the 
> application side.
> I think that it is useful to determine whether the cause is on the 
> application side or the server side when a slow query occurs. 

Do you mean you want to monitor the protocol message exchange at
the client side to analyze performance issues, right?  Actually,
this might be useful to determin where is the problem, for example,
the client application, the backend PostgreSQL, or somewhere in the
network.

Such logging can be implemented in the application, but if libpq
provides the standard way, it would be helpful to resolve a problem
without modifying the application code.

> The provided information is "date and time" at which execution of processing 
> is started, "query", "application side processing", which is picked up 
> information from PQtrace(), and "connection id", which is for uniquely 
> identifying the connection. 

I couldn't image how this is like. Could you show us a sample of log lines
in your head?

> To collect the log, set the connection string or environment variable.
> - logdir or PGLOGDIR : directory where log file written
> - logsize or PGLOGSIZE : maximum log size

How we can specify the log file name?  What should happen if a file size
exceeds to PGLOGSIZE?

Regards,
-- 
Yugo Nagata 

Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

[Yugo Nagata]

On Mon, 27 Aug 2018 13:34:12 +0200
Michael Banck  wrote:

> Hi,
> 
> On Mon, Aug 27, 2018 at 07:53:36PM +0900, Yugo Nagata wrote:
> > On Fri, 24 Aug 2018 18:01:09 +0200
> > Peter Eisentraut  wrote:
> > > I'm curious about this option:
> > > 
> > >   -r RELFILENODE check only relation with specified relfilenode
> > > 
> > > but there is no facility to specify a database.
> > > 
> > > Also, referring to the relfilenode of a mapped relation seems a bit
> > > inaccurate.
> > > 
> > > Maybe reframing this in terms of the file name of the file you want
> > > checked would be better?
> > 
> > If we specified 1234 to -r option, pg_verify_shceksums checks not only 1234
> > but also 1234_vm, 1234_fsm, and 1234.1, 1234.2, ... and so on, so I think
> > it makes senses to allow to specify a relfilenode instead of a file name.
> > 
> > I think it is reasonable to add a option to specify a database, although
> > I don't know which character is good because both -d and -D are already 
> > used
> 
> Maybe the -d (debug) option should be revisited as well. Mentioning
> every scanned block generates a huge amount of output which might be
> useful during development but does not seem very useful for a stable
> release. AFAICT there is no other debug output for now.
> 
> So it could be renamed to -v (verbose) and only mention each scanned
> file, e.g. (errors/checksum mismatches are still reported of course).
> 
> Then -d could (in the future, I guess that is too late for v11) be used
> for -d/--dbname (or make that only a long option, if the above does not
> work).

I realized after sending the previous post that we can not specify a database
by name because pg_verify_checksum is run in offline and this can not get the
OID from the database name.  Also, there are global and pg_tblspc directories
not only base/. So, it seems to me good to specify a directories
to scan which is under PGDATA. We would be able to use -d ( or --directory ?)
for this purpose.


Regards,
-- 
Yugo Nagata 

Re: pg_verify_checksums -r option

[Yugo Nagata]

On Fri, 24 Aug 2018 18:01:09 +0200
Peter Eisentraut  wrote:

> I'm curious about this option:
> 
>   -r RELFILENODE check only relation with specified relfilenode
> 
> but there is no facility to specify a database.
> 
> Also, referring to the relfilenode of a mapped relation seems a bit
> inaccurate.
> 
> Maybe reframing this in terms of the file name of the file you want
> checked would be better?

If we specified 1234 to -r option, pg_verify_shceksums checks not only 1234
but also 1234_vm, 1234_fsm, and 1234.1, 1234.2, ... and so on, so I think
it makes senses to allow to specify a relfilenode instead of a file name.

I think it is reasonable to add a option to specify a database, although
I don't know which character is good because both -d and -D are already used

Regards,
-- 
Yugo Nagata 

Re: Refactor textToQualifiedNameList()

[Yugo Nagata]

On Fri, 24 Aug 2018 20:44:12 +0900
Yugo Nagata  wrote:

> Hi,
> 
> When working on other patch[1], I found there are almost same
> functions, texttoQualifiedNameList() and stringToQualifiedNameList().
> The only difference is the argument type, text or char*. I don't know
> why these functions are defined seperately, but I think the former 
> function can be rewritten using the latter code as the attached patch.
> Is this reasonable fix?

I forgot to add a link to the referred thread.

[1] 
https://www.postgresql.org/message-id/20180817075100.bc99378255943d3c3951ad63%40sraoss.co.jp

> 
> Regards,
> -- 
> Yugo Nagata 


-- 
Yugo Nagata 

Re: has_table_privilege for a table in unprivileged schema causes an error

[Yugo Nagata]

On Sat, 25 Aug 2018 23:29:27 -0400
Tom Lane  wrote:

> Robert Haas  writes:
> > I'm not sure that it's a good idea to change this behavior.
> 
> > In the case of an unqualified name, the permissions on the schemas in
> > the search path can affect which table is chosen in the first place.
> > ... So I think this only matters for qualified names.
> 
> Yeah, that agrees with my expectations.

Yes. I consider only the cases of qualified names and the patch doesn't 
change any behavior about unqualified name cases.

> > Also, the current system generally tries not to reveal any information
> > about the contents of schemas for which you have no permissions.
> 
> I don't think that argument holds up, at least not if this is implemented
> the way I'd expect.  It would change the results for a schema on which
> you lack usage permission from "permission denied for schema a" to
> "false", but it still would not matter whether there is such a table
> inside "a".

Yes, Tom's explanation is right. The proposal functions doesn't reveal
any information about the contents of unprivileged schemas, either.

> 
> > And if you've got a qualified name, you know what schema it's in.  If
> > you are concerned about a.b, nothing keeps you from writing a test
> > against schema a's permissions as well as a test against table a.b's
> > permissions.  But on the other hand, if for some reason you want to
> > know about pg_class.relacl specifically, then having the function
> > consider both that and the schema's ACL could be awkward.
> 
> Mmm ... maybe, but I don't think that exactly holds water either, given
> that the current behavior is to fail if you lack permission on schema a.
> Yes, you can write "case when has_schema_privilege() then
> has_table_privilege() else false end", but if you're worried that you
> might possibly lack schema privilege, then the current behavior of
> has_table_privilege is useless to you: it doesn't matter whether or not
> you would like to know about pg_class.relacl specifically, because you
> won't be allowed to find out.
> 
> Also, in some use-cases the extra test would require writing code that can
> split a qualified name into pieces, which isn't really all that easy in
> SQL.

This is a reason why we proposed to fix the function. However, with regard to
splitting a qualified name, making a new SQL function to do this might resolve
it, for example, as below.
 
 select case when has_schema_privilege(x.nspname) 
   then has_table_privilege(x.objname)
   else false end
  from pg_split_qualified_name(tablename) x;

> There's a backwards-compatibility argument for not changing this behavior,
> sure, but I don't buy the other arguments you made here.  And I don't
> think there's all that much to the backwards-compatibility argument,
> considering that the current behavior is to fail.

With regarding to keeping the backwards-compatibility, to add a new paramater
to has_*_privilege functions is a solution as proposed previously.

 has_table_privilege(user, table, privilege[, consider_schema=false]) 

How do you think this proposal?

Regards,

-- 
Yugo Nagata 

Refactor textToQualifiedNameList()

[Yugo Nagata]

Hi,

When working on other patch[1], I found there are almost same
functions, texttoQualifiedNameList() and stringToQualifiedNameList().
The only difference is the argument type, text or char*. I don't know
why these functions are defined seperately, but I think the former 
function can be rewritten using the latter code as the attached patch.
Is this reasonable fix?

Regards,
-- 
Yugo Nagata 
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index a5e812d026..2ef1a1e330 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -34,6 +34,7 @@
 #include "utils/memutils.h"
 #include "utils/pg_locale.h"
 #include "utils/sortsupport.h"
+#include "utils/regproc.h"
 #include "utils/varlena.h"
 
 
@@ -3233,34 +3234,12 @@ textToQualifiedNameList(text *textval)
 {
 	char	   *rawname;
 	List	   *result = NIL;
-	List	   *namelist;
-	ListCell   *l;
 
 	/* Convert to C string (handles possible detoasting). */
 	/* Note we rely on being able to modify rawname below. */
 	rawname = text_to_cstring(textval);
 
-	if (!SplitIdentifierString(rawname, '.', ))
-		ereport(ERROR,
-(errcode(ERRCODE_INVALID_NAME),
- errmsg("invalid name syntax")));
-
-	if (namelist == NIL)
-		ereport(ERROR,
-(errcode(ERRCODE_INVALID_NAME),
- errmsg("invalid name syntax")));
-
-	foreach(l, namelist)
-	{
-		char	   *curname = (char *) lfirst(l);
-
-		result = lappend(result, makeString(pstrdup(curname)));
-	}
-
-	pfree(rawname);
-	list_free(namelist);
-
-	return result;
+	return stringToQualifiedNameList(rawname);
 }
 
 /*

Re: has_table_privilege for a table in unprivileged schema causes an error

[Yugo Nagata]

Hi,

I updated the patch to support other has_*_privilege functions with some
refactoring, but tests for theses fixes are not included yet.

But, while running the regression test after this patch is applied, I found
that my patch doesn't pass privilege test.  One of different behaviours
is as bellow.

 -- Grant on all objects of given type in a schema
 \c -
 
 CREATE SCHEMA testns;
 CREATE TABLE testns.t1 (f1 int);
 CREATE TABLE testns.t2 (f1 int);
 
 SELECT has_table_privilege('regress_priv_user1', 'testns.t1', 'SELECT'); -- 
false
 
 GRANT ALL ON ALL TABLES IN SCHEMA testns TO regress_priv_user1;
 
 SELECT has_table_privilege('regress_priv_user1', 'testns.t1', 'SELECT'); -- 
true

This is a part of src/test/regress/sql/privileges.sql. SELECT privilege on 
testns.t1 
is granted to regress_priv_user1, so the has_table_privilege of the original 
implementation
returns true.  On the other hand, after applied my patch, the function returns 
false
because the regress_priv_user1 doesn't have USAGE privilege on schema testns. 
Accually, 
the user can not access to the table using SELECT statement. 

Although the behavior of the original function would reflect pg_class.relacl, 
it seems to
me that the function fixed in my patch is more useful for users because this 
reflects
the actual accessibility during query execution.

Is there chance to change the behaviour of the functions as I am proposing?

If is is not allowed to change it to keep back-compatibility, I would like to 
propose
to add a parameter to the function to consider the privilege of the schema, for
example as bellow. Assuming false as the default values will keep the 
back-compatibility.

 has_table_privilege(user, table, privilege[, consider_schema=false]) 

On Fri, 17 Aug 2018 12:02:57 +0900
Yugo Nagata  wrote:

> On Thu, 16 Aug 2018 19:37:42 -0400
> Tom Lane  wrote:
> 
> > Yugo Nagata  writes:
> > > I found that has_table_privilege returns an error when a table is 
> > > specified
> > > by schema-qualified name and the user doen't have privilege for its 
> > > schema.
> > 
> > >  postgres=> select has_table_privilege('myschema.tbl','select');
> > >  ERROR:  permission denied for schema myschema
> > 
> > > I think that this function should return false because the user doesn't 
> > > have
> > > the privilege on this table eventually.  It is more useful for users 
> > > because
> > > it is not needed to parse the schema-qualified table name and check the
> > > privilege on the schema in advance.
> > 
> > Sounds reasonable, but if we're going to do that, we should do it for
> > every one of these functions that concerns a schema-qualifiable object
> > type.  Not just tables.
> 
> OK. I will fix all of these functions that can take a schema-qualifiable
> object name as a parameter.

In addition to has_table_privilege, has_any_column_privilege, 
has_column_privilege,
has_function_privilege, and has_sequence_privilege are fixed.

> > 
> > Also, looking at the code, why are you bothering with
> > convert_table_schema_priv_string?  ISTM what's relevant on the schema is
> > always going to be USAGE privilege, independently of the mode being
> > checked on the object.  So you shouldn't need a bunch of duplicative
> > tables.
> 
> I thought we needed to consider also USAGE GRANT OPTION, but I might be
> misunderstnding something. I will look into this again.

Fixed to check only USAGE privilege on the schema.

> > Plus, I don't think this implementation approach is going to work for
> > unqualified table names.  You don't know which schema they're in until you
> > look them up.  (Although I vaguely remember that the path search logic just
> > ignores unreadable schemas, so maybe all you have to do with unqualified
> > names is nothing.  But that's not what this patch is doing now.)
> 
> Oops. I overlooked these cases.  I'll fix the patch to allow to handle
> unqualified table names.

Fixed.

Regards,
-- 
Yugo Nagata 
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index a45e093de7..262472e424 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -94,6 +94,7 @@ static AclMode convert_priv_string(text *priv_type_text);
 static AclMode convert_any_priv_string(text *priv_type_text,
 		const priv_map *privileges);
 
+static bool check_schema_usage_privilege(text *objectname, Oid roleid);
 static Oid	convert_table_name(text *tablename);
 static AclMode convert_table_priv_string(text *priv_type_text);
 static AclMode convert_sequence_priv_string(text *priv_type_text);
@@ -1860,6 +1861,10 @@ has_table_privilege_name_name(PG_FUNCTION_ARGS)
 	AclResult	aclresult;
 
 	roleid = get_role_oid_or_public(NameStr(*rolename));
+
+	if (!check_schema_usage_privilege(ta

Re: has_table_privilege for a table in unprivileged schema causes an error

[Yugo Nagata]

On Thu, 16 Aug 2018 19:37:42 -0400
Tom Lane  wrote:

> Yugo Nagata  writes:
> > I found that has_table_privilege returns an error when a table is specified
> > by schema-qualified name and the user doen't have privilege for its schema.
> 
> >  postgres=> select has_table_privilege('myschema.tbl','select');
> >  ERROR:  permission denied for schema myschema
> 
> > I think that this function should return false because the user doesn't have
> > the privilege on this table eventually.  It is more useful for users because
> > it is not needed to parse the schema-qualified table name and check the
> > privilege on the schema in advance.
> 
> Sounds reasonable, but if we're going to do that, we should do it for
> every one of these functions that concerns a schema-qualifiable object
> type.  Not just tables.

OK. I will fix all of these functions that can take a schema-qualifiable
object name as a parameter.

> 
> Also, looking at the code, why are you bothering with
> convert_table_schema_priv_string?  ISTM what's relevant on the schema is
> always going to be USAGE privilege, independently of the mode being
> checked on the object.  So you shouldn't need a bunch of duplicative
> tables.

I thought we needed to consider also USAGE GRANT OPTION, but I might be
misunderstnding something. I will look into this again.
 
> Plus, I don't think this implementation approach is going to work for
> unqualified table names.  You don't know which schema they're in until you
> look them up.  (Although I vaguely remember that the path search logic just
> ignores unreadable schemas, so maybe all you have to do with unqualified
> names is nothing.  But that's not what this patch is doing now.)

Oops. I overlooked these cases.  I'll fix the patch to allow to handle
unqualified table names.

Thanks,
 
-- 
Yugo Nagata 

has_table_privilege for a table in unprivileged schema causes an error

[Yugo Nagata]

Hi,

I found that has_table_privilege returns an error when a table is specified
by schema-qualified name and the user doen't have privilege for its schema.

 postgres=> select has_table_privilege('myschema.tbl','select');
 ERROR:  permission denied for schema myschema

I think that this function should return false because the user doesn't have
the privilege on this table eventually.  It is more useful for users because
it is not needed to parse the schema-qualified table name and check the
privilege on the schema in advance.

Attached is a patch to modify the function like that. This is WIP patch, so
only has_table_previlege is modified and other familiy functions are left as
they are. Also, there is no additional test yet.

One consern on this patch is that modifying the function can break the 
back-compatibility, so it might be better to add a new parameter to
control the behavior of the function. 

Any comments would be appriciated.

Regards,
-- 
Yugo Nagata 
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index a45e093de7..6628385277 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -95,7 +95,9 @@ static AclMode convert_any_priv_string(text *priv_type_text,
 		const priv_map *privileges);
 
 static Oid	convert_table_name(text *tablename);
+static Oid	convert_table_schema_name(text *tablename);
 static AclMode convert_table_priv_string(text *priv_type_text);
+static AclMode convert_table_schema_priv_string(text *priv_type_text);
 static AclMode convert_sequence_priv_string(text *priv_type_text);
 static AttrNumber convert_column_name(Oid tableoid, text *column);
 static AclMode convert_column_priv_string(text *priv_type_text);
@@ -1856,10 +1858,19 @@ has_table_privilege_name_name(PG_FUNCTION_ARGS)
 	text	   *priv_type_text = PG_GETARG_TEXT_PP(2);
 	Oid			roleid;
 	Oid			tableoid;
+	Oid			schemaoid;
 	AclMode		mode;
 	AclResult	aclresult;
 
 	roleid = get_role_oid_or_public(NameStr(*rolename));
+
+	schemaoid = convert_table_schema_name(tablename);
+	mode = convert_table_schema_priv_string(priv_type_text);
+
+	aclresult = pg_namespace_aclcheck(schemaoid, roleid, mode);
+	if (aclresult != ACLCHECK_OK)
+		PG_RETURN_BOOL(false);
+
 	tableoid = convert_table_name(tablename);
 	mode = convert_table_priv_string(priv_type_text);
 
@@ -1881,10 +1892,19 @@ has_table_privilege_name(PG_FUNCTION_ARGS)
 	text	   *priv_type_text = PG_GETARG_TEXT_PP(1);
 	Oid			roleid;
 	Oid			tableoid;
+	Oid			schemaoid;
 	AclMode		mode;
 	AclResult	aclresult;
 
 	roleid = GetUserId();
+
+	schemaoid = convert_table_schema_name(tablename);
+	mode = convert_table_schema_priv_string(priv_type_text);
+
+	aclresult = pg_namespace_aclcheck(schemaoid, roleid, mode);
+	if (aclresult != ACLCHECK_OK)
+		PG_RETURN_BOOL(false);
+
 	tableoid = convert_table_name(tablename);
 	mode = convert_table_priv_string(priv_type_text);
 
@@ -1957,9 +1977,17 @@ has_table_privilege_id_name(PG_FUNCTION_ARGS)
 	text	   *tablename = PG_GETARG_TEXT_PP(1);
 	text	   *priv_type_text = PG_GETARG_TEXT_PP(2);
 	Oid			tableoid;
+	Oid			schemaoid;
 	AclMode		mode;
 	AclResult	aclresult;
 
+	schemaoid = convert_table_schema_name(tablename);
+	mode = convert_table_schema_priv_string(priv_type_text);
+
+	aclresult = pg_namespace_aclcheck(schemaoid, roleid, mode);
+	if (aclresult != ACLCHECK_OK)
+		PG_RETURN_BOOL(false);
+
 	tableoid = convert_table_name(tablename);
 	mode = convert_table_priv_string(priv_type_text);
 
@@ -1996,6 +2024,20 @@ has_table_privilege_id_id(PG_FUNCTION_ARGS)
  *		Support routines for has_table_privilege family.
  */
 
+/*
+ * Given a table name expressed as a string, return its schema Oid
+ */
+static Oid
+convert_table_schema_name(text *tablename)
+{
+	RangeVar   *relrv;
+
+	relrv = makeRangeVarFromNameList(textToQualifiedNameList(tablename));
+
+	return get_namespace_oid(relrv->schemaname, false);
+}
+
+
 /*
  * Given a table name expressed as a string, look it up and return Oid
  */
@@ -2040,6 +2082,36 @@ convert_table_priv_string(text *priv_type_text)
 	return convert_any_priv_string(priv_type_text, table_priv_map);
 }
 
+/*
+ * convert_table_schema_priv_string
+ *		Convert text string to AclMode value.
+ */
+static AclMode
+convert_table_schema_priv_string(text *priv_type_text)
+{
+	static const priv_map table_priv_map[] = {
+		{"SELECT", ACL_USAGE},
+		{"SELECT WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_USAGE)},
+		{"INSERT", ACL_USAGE},
+		{"INSERT WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_USAGE)},
+		{"UPDATE", ACL_USAGE},
+		{"UPDATE WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_USAGE)},
+		{"DELETE", ACL_USAGE},
+		{"DELETE WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_USAGE)},
+		{"TRUNCATE", ACL_USAGE},
+		{"TRUNCATE WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_USAGE)},
+		{"REFERENCES", ACL_USAGE},
+		{"REFERENCES WITH GRANT OPTION", ACL_GRANT_OPTIO

Re: Fw: Windows 10 got stuck with PostgreSQL at starting up. Adding delay lets it avoid.

[Yugo Nagata]

On Fri, 20 Jul 2018 10:48:15 -0400
Tom Lane  wrote:

> Yugo Nagata  writes:
> > Recently, one of our clients reported a problem that Windows 10 sometime 
> > (approximately once in 300 tries) hung up at OS starting up while PostgreSQL
> > 9.3.x service is starting up. My co-worker analyzed this and found that
> > PostgreSQL's auxiliary process and Windows' logon processes are in a 
> > dead-lock
> > situation.
> 
> Really?  What would they deadlock on?  Why is there any connection
> whatsoever?  Why has nobody else run into this?

It is not clear where the hang occered, but this might be a problem
only on the specific version of Windows. Our client reported that
the hang occured with  Windows 10 IoT Enterpise 2015 LTSB, but not
with Windows 10 IoT Enterpise 2016 LTSB or Windows 7. 

> 
> > He reported this problem to pgsql-general list as below. Also, he created a 
> > patch
> > to add a build-time option for adding 0.5 or 3.0 seconds delay after each 
> > sub 
> > process starts.
> 
> This seems like an ugly hack that probably doesn't reliably resolve
> whatever the problem is, but does manage to kill postmaster
> responsiveness :-(.  It'd be especially awful to insert such a delay
> after forking parallel worker processes, which would be a problem in
> anything much newer than 9.3.

Agreed.

> I think we need more investigation; and to start with, reproducing
> the problem in a branch that's not within hailing distance of its EOL
> would be a good idea.  (Not that I have reason to think PG's behavior
> has changed much here ... but 9.3 is just not a good basis for asking
> us to do anything now.)

They also reported that this problem occured with Windows 10 IoT Enterpise
2015 LTSB + PostgreSQL 10.3 as well as PostgreSQL 9.3.22. However, 
reproducing this would be hard because we don't have Windows 10 IoT
enviromnemt and also the frequency is approximately once in 300 retries
of OS startup.

We will investigate this more and report if we found something.

Regards,


-- 
Yugo Nagata 

Re: Fw: Windows 10 got stuck with PostgreSQL at starting up. Adding delay lets it avoid.

[Yugo Nagata]

On Fri, 20 Jul 2018 19:13:21 +0900
Michael Paquier  wrote:

> On Fri, Jul 20, 2018 at 05:58:13PM +0900, Yugo Nagata wrote:
> > He reported this problem to pgsql-general list as below. Also, he created a 
> > patch
> > to add a build-time option for adding 0.5 or 3.0 seconds delay after each 
> > sub 
> > process starts.  The attached is the same one.  Our client confirmed that 
> > this 
> > patch resolves the dead-lock problem. Is it acceptable to add this option 
> > to 
> > PostgreSQL?  Any comment would be appreciated.
> 
> If the OS startup gets slower, then an arbitrary delay is not going to
> solve things and you would finish by facing the same problem.  It seems
> to me that we need to understand what are the low-level locks which get
> stuck, if it is possible to make their acquirement conditional, and then
> loop conditionally with multiple retries.

>From investigation of the kernel dump of Windows, it seems that PushLocks
were acqired in postgres processes and that this caused the deadlock.
However, it is still not clear which part of postgres code is involved this
lock. We will investigate this more and report if we found something.

> --
> Michael


-- 
Yugo Nagata 

Fw: Windows 10 got stuck with PostgreSQL at starting up. Adding delay lets it avoid.

[Yugo Nagata]

Hi,

Recently, one of our clients reported a problem that Windows 10 sometime 
(approximately once in 300 tries) hung up at OS starting up while PostgreSQL
9.3.x service is starting up. My co-worker analyzed this and found that
PostgreSQL's auxiliary process and Windows' logon processes are in a dead-lock
situation.

Although this problem have been found only with PostgreSQL 9.3.x and Windows 10
in our client's environment for now, maybe the same problem occurs with other 
versions of PostgreSQL.

He reported this problem to pgsql-general list as below. Also, he created a 
patch
to add a build-time option for adding 0.5 or 3.0 seconds delay after each sub 
process starts.  The attached is the same one.  Our client confirmed that this 
patch resolves the dead-lock problem. Is it acceptable to add this option to 
PostgreSQL?  Any comment would be appreciated.

Regards,




Begin forwarded message:

Date: Fri, 29 Jun 2018 15:03:10 +0900
From: TAKATSUKA Haruka 
To: pgsql-gene...@postgresql.org
Subject: Windows 10 got stuck with PostgreSQL at starting up. Adding delay lets 
it avoid.


I got a trouble in PostgreSQL 9.3.x on Windows 10.
I would like to add new delay code as an official build option.

Windows 10 sometime (approximately once in 300 tries) hung up 
at OS starting up. The logs say it happened while the PostgreSQL 
service was starting. When OS stopped, some postgres auxiliary 
process were started and some were not started yet. 

The Windows dump say some threads of the postgres auxiliary process
are waiting OS level locks and the logon processes’thread are
also waiting a lock. MS help desk said that PostgreSQL’s OS level 
deadlock caused OS freeze. I think it is strange story. But, 
in fact, it not happened in repeated tests when I got rid of 
PostgreSQL from the initial auto-starting services.

I tweaked PostgreSQL 9.3.x (the newest from the repository) to add 
0.5 or 3.0 seconds delay after each sub process starts. 
And then the hung up was gone. This test patch is attached. 
It is only implemented for Windows. Also, I did not use existing 
pg_usleep because it contains locking codes (e.g. WaitForSingleObject
and Enter/LeaveCriticalSection).

Although Windows OS may have some problems, I think we should have
a means to avoid it. Can PostgreSQL be accepted such delay codes
as build-time options by preprocessor variables?


Thanks,
Takatsuka Haruka


-- 
Yugo Nagata 
diff --git a/src/backend/postmaster/postmaster.c 
b/src/backend/postmaster/postmaster.c
index d6fc2ed..ff03ebd 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -398,6 +398,30 @@ extern int optreset;   /* might not be 
declared by system headers */
 static DNSServiceRef bonjour_sdref = NULL;
 #endif
 
+#define USE_AFTER_AUX_FORK_SLEEP 3000
+
+#ifdef USE_AFTER_AUX_FORK_SLEEP
+#ifndef WIN32
+#define AFTER_AUX_FORK_SLEEP()
+#else
+#define AFTER_AUX_FORK_SLEEP() do { SleepEx(USE_AFTER_AUX_FORK_SLEEP, FALSE); 
} while(0)
+#endif
+#else
+#define AFTER_AUX_FORK_SLEEP()
+#endif
+
+#define USE_AFTER_BACKEND_FORK_SLEEP 500
+
+#ifdef USE_AFTER_BACKEND_FORK_SLEEP
+#ifndef WIN32
+#define AFTER_BACKEND_FORK_SLEEP()
+#else
+#define AFTER_BACKEND_FORK_SLEEP() do { SleepEx(USE_AFTER_BACKEND_FORK_SLEEP, 
FALSE); } while(0)
+#endif
+#else
+#define AFTER_BACKEND_FORK_SLEEP()
+#endif
+
 /*
  * postmaster.c - function prototypes
  */
@@ -1709,6 +1733,7 @@ ServerLoop(void)
 */
StreamClose(port->sock);
ConnFree(port);
+   AFTER_BACKEND_FORK_SLEEP();
}
}
}
@@ -2801,11 +2826,20 @@ reaper(SIGNAL_ARGS)
 * situation, some of them may be alive already.
 */
if (!IsBinaryUpgrade && AutoVacuumingActive() && 
AutoVacPID == 0)
+   {
AutoVacPID = StartAutoVacLauncher();
+   AFTER_AUX_FORK_SLEEP(); 
+   }
if (XLogArchivingActive() && PgArchPID == 0)
+   {
PgArchPID = pgarch_start();
+   AFTER_AUX_FORK_SLEEP();
+   }
if (PgStatPID == 0)
+   {
PgStatPID = pgstat_start();
+   AFTER_AUX_FORK_SLEEP();
+   }
 
/* some workers may be scheduled to start now */
maybe_start_bgworker();
@@ -5259,6 +5293,7 @@ StartChildProcess(AuxProcType type)
/*
 * in parent, successful fork
 */
+   AFTER_AUX_FORK_SLEEP();
return pid;
 }
 

Re: Temporary WAL segments files not cleaned up after an instance crash

[Yugo Nagata]

On Thu, 12 Jul 2018 16:44:45 +0900
Michael Paquier  wrote:

> On Thu, Jul 12, 2018 at 03:35:53PM +0900, Yugo Nagata wrote:
> > I think it makes sense to remove unnecessary temporary WAL files although
> > I'm not sure how high the risk of ENOSPC is.
> 
> It depends on how close to the partition size limit max_wal_size is set,
> and how much a system is unstable.  Switching on/off a VM where Postgres
> is located can participate in that, as well as VM snapshots taken
> without memory (I work a lot on those as you can guess :D).  Setting it
> to 70% of the partition size is what I imagine is the base, but I can
> imagine as well people setting it at 90% or more.

Thank you for your explaining this. I have understood the problem
you concern well.

Thanks,

-- 
Yugo Nagata 

Re: Problem on pg_dump RANGE partition with expressions

[Yugo Nagata]

On Thu, 12 Jul 2018 17:44:48 +0900
Amit Langote  wrote:

> > 1) Allow to appear more than once in range partition key
> > 
> > I don't understand why there is this restriction. If we have no clear 
> > reason, 
> > can we rip out this restrition?
> 
> I can't recall exactly, but back when I wrote this code, I might have been
> thinking that such a feature is useless and would actually just end up
> being a foot gun for users.
> 
> Although, I'm open to ripping it out if it's seen as being overly restrictive.

I think this way is good to resolve this, because allowing columns to appear 
more
than once would be harmless at least with the current partitioning methods, 
range and hash, although this is useless. 

Actually, we currenly allow same expression apears more than once in partition 
key
like

 create table t (i int) partition by range((i+1),(i+1));

In the past, I proposed a patch to forbid this, but this is rejected
since there is harmless and no need to restrict this.

Attached is a patch to get rid of "appears more than once" restriction.

Regards,

-- 
Yugo Nagata 
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 22e81e7..d7e8633 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13623,20 +13623,6 @@ transformPartitionSpec(Relation rel, PartitionSpec *partspec, char *strategy)
 		PartitionElem *pelem = castNode(PartitionElem, lfirst(l));
 		ListCell   *lc;
 
-		/* Check for PARTITION BY ... (foo, foo) */
-		foreach(lc, newspec->partParams)
-		{
-			PartitionElem *pparam = castNode(PartitionElem, lfirst(lc));
-
-			if (pelem->name && pparam->name &&
-strcmp(pelem->name, pparam->name) == 0)
-ereport(ERROR,
-		(errcode(ERRCODE_DUPLICATE_COLUMN),
-		 errmsg("column \"%s\" appears more than once in partition key",
-pelem->name),
-		 parser_errposition(pstate, pelem->location)));
-		}
-
 		if (pelem->expr)
 		{
 			/* Copy, to avoid scribbling on the input */

Re: Preferring index-only-scan when the cost is equal

[Yugo Nagata]

On Thu, 12 Jul 2018 12:59:15 +0200
Tomas Vondra  wrote:

> 
> 
> On 07/12/2018 03:44 AM, Yugo Nagata wrote:
> > On Wed, 11 Jul 2018 14:37:46 +0200
> > Tomas Vondra  wrote:
> > 
> >>
> >> On 07/11/2018 01:28 PM, Ashutosh Bapat wrote:
> > 
> >>> I don't think we should change add_path() for this. We will
> >>> unnecessarily check that condition even for the cases where we do not
> >>> create index paths. I think we should fix the caller of add_path()
> >>> instead to add index only path before any index paths. For that the
> >>> index list needs to be sorted by the possibility of using index only
> >>> scan.
> >>>
> >>> But I think in your case, it might be better to first check whether
> >>> there is any costing error because of which index only scan's path has
> >>> the same cost as index scan path. Also I don't see any testcase which
> >>> will show why index only scan would be more efficient in your case.
> >>> May be provide output of EXPLAIN ANALYZE.
> >>>
> >>
> >> I suspect this only happens due to testing on empty tables. Not only is
> >> testing of indexes on small tables rather pointless in general, but more
> >> importantly there will be no statistics. So we fall back to some default
> >> estimates, but we also don't have relallvisible etc which is crucial for
> >> estimating index-only scans. I'd bet that's why the cost estimates for
> >> index scans and index-only scans are the same here.
> > 
> > You are right. When the table have rows and this is vacuumed, index only
> > scan's cost is cheaper and chosen properly. Sorry, I have jumped to the
> > conclusion before confirming this.
> > 
> 
> I'm very experienced in this. I've done this mistake a million times ;-)

Thank you. It is really encouraging for me.

> 
> regards
> 
> -- 
> Tomas Vondra  http://www.2ndQuadrant.com
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Yugo Nagata 

Problem on pg_dump RANGE partition with expressions

[Yugo Nagata]

Hi,

During looking into other thread[1], I found a problem on pg_dump of range
partition table using expressions. When we create a range partitioned table, 
we cannot use a column more than　once in the partition key.
 
 postgres=# create table t (i int) partition by range(i,i);
 ERROR:  column "i" appears more than once in partition key

On the other hand, the following query using expression is allowed.

 postgres=# create table test (i int) partition by range(i,(i));
 CREATE TABLE

However, when we use pg_dump for this, we get

 CREATE TABLE public.test (
 i integer
 )
 PARTITION BY RANGE (i, i);

, and we cannot restore this due to the error.

I can consider three approaches to resolve this.


1) Allow to appear more than once in range partition key

I don't understand why there is this restriction. If we have no clear reason, 
can we rip out this restrition?

2) Forbid this kind of partition key

If we can make more checks and forbid such partition keys as RANGE(i,(i)), 
we can avoid the problem though it is a bit rigorous.

3) Treat expressions as it is

For some reasons, expressions like "(column)" or "(column COLLATE something)"
is treated like simple attributes in the current implementation
(in ComputePartitionAttr() ).

If we treat these expressions as it is, pg_dump result will be the same 
expression as when the partition table is created, and we can restore this 
successfully.

Could you give me your opinions aoubt which approach is appropriate?

Regards,

[1] 
https://www.postgresql.org/message-id/flat/20180712155808.49e712d8.nagata%40sraoss.co.jp#00bbfb5054c0a57f9a2fe48fae77b848

-- 
Yugo Nagata 

Re: [PG-11] Potential bug related to INCLUDE clause of CREATE INDEX

[Yugo Nagata]

On Thu, 12 Jul 2018 15:58:08 +0900
Yugo Nagata  wrote:
 
> Yes, more simplly, the following query also works;
> 
>  CREATE INDEX ON test((i)) INCLUDE (i);
> 
> However, a problem is that when we use pg_dump for the database, this 
> generate the following query
> 
>  CREATE INDEX test_i_i1_idx ON public.test USING btree (i) INCLUDE (i);
> 
> Of cause, this causes the "must not intersect" error, and we cannot restore 
> this dump.
> 
> To fix this, we agree with Tom about getting rid of "must not intersect" 
> restriction.
> A patch is attached for this

Should we add this to PG11 open items?




-- 
Yugo Nagata 

Re: Temporary WAL segments files not cleaned up after an instance crash

[Yugo Nagata]

On Mon, 14 May 2018 14:49:55 +0900
Michael Paquier  wrote:

> Hi all,
> 
> While playing with a standby as follows I noticed that xlogtemp.*
> generated in pg_wal may stay around when entering crash recovery.  The
> test I was conducting is pretty simple:
> - Use a primary and a standby.
> - Run pgbench on the primary.
> - Then restart the standby with -m immediate and force WAL segment
> switch on the primary in a loop.  Depending on the timing, one can see
> that those xlogtemp files stay around.  Those files are here when
> creating a new segment from scratch and append the PID of the process
> creating them.  Any previous file existing with the same name is
> unlinked.
> 
> The problem is that if an instance is not really stable for a reason or
> another and starts crash recovery periodically, then there is a risk of
> accumulating those temporary files.  If pg_wal is on its own partition,
> tuned by max_wal_size, then there is a risk to run into ENOSPC and take
> PostgreSQL down as new WAL segments cannot be created.
> 
> Shouldn't those files be cleaned up at the beginning of crash recovery?
> Attached is a proposal of patch doing so.

I think it makes sense to remove unnecessary temporary WAL files although
I'm not sure how high the risk of ENOSPC is. 

The code looks fine, the patch can be applied to HEAD, and I can build this
successfully. I confirmed that all tempxlog.* files are removed when restarting
postgres that was shutdown immediately. 

One little thing I noticed is the function name "RemoveXLogTempFiles". 
Other similar functions are named as RemoveOldXlogFiles or RemoveXlogFile
(using Xlog not XLog), so it seem to me more consistent to rename this 
"RemoveXlogTempFiles" or "RemoveTempXlogFiles" and so on.

Regards




-- 
Yugo Nagata 

Re: Preferring index-only-scan when the cost is equal

[Yugo Nagata]

On Wed, 11 Jul 2018 14:37:46 +0200
Tomas Vondra  wrote:

> 
> On 07/11/2018 01:28 PM, Ashutosh Bapat wrote:

> > I don't think we should change add_path() for this. We will
> > unnecessarily check that condition even for the cases where we do not
> > create index paths. I think we should fix the caller of add_path()
> > instead to add index only path before any index paths. For that the
> > index list needs to be sorted by the possibility of using index only
> > scan.
> > 
> > But I think in your case, it might be better to first check whether
> > there is any costing error because of which index only scan's path has
> > the same cost as index scan path. Also I don't see any testcase which
> > will show why index only scan would be more efficient in your case.
> > May be provide output of EXPLAIN ANALYZE.
> > 
> 
> I suspect this only happens due to testing on empty tables. Not only is 
> testing of indexes on small tables rather pointless in general, but more 
> importantly there will be no statistics. So we fall back to some default 
> estimates, but we also don't have relallvisible etc which is crucial for 
> estimating index-only scans. I'd bet that's why the cost estimates for 
> index scans and index-only scans are the same here.

You are right. When the table have rows and this is vacuumed, index only
scan's cost is cheaper and chosen properly. Sorry, I have jumped to the
conclusion before confirming this.

Thanks,

-- 
Yugo Nagata 

Re: Allow to specify a index name as ANALYZE parameter

[Yugo Nagata]

On Wed, 11 Jul 2018 14:26:03 +0300
Alexander Korotkov  wrote:
 
> On Wed, Jul 11, 2018 at 12:04 PM Yugo Nagata  wrote:
> > When we specify column names for ANALYZE, only the statistics for those 
> > columns
> > are collected. Similarly, is it useful if we have a option to specify an 
> > index
> > for ANALYZE to collect only the statistics for expression in the specified 
> > index?
> >
> > A usecase I suppose is that when a new expression index is created and that
> > we need only the statistics for the new index. Although ANALYZE of all the 
> > indexes
> > is usually fast because ANALYZE uses a random sampling of the table rows, 
> > ANALYZE
> > on the specific index may be still useful if there are other index whose 
> > "statistics
> > target" is large and/or whose expression takes time to compute, for example.
> >
> > Attached is the WIP patch to allow to specify a index name as ANALYZE 
> > parameter.
> > Any documatation is not yet included.  I would appreciate any feedback!
> 
> I think this makes sense.  Once we can collect statistics individually
> for regular columns, we should be able to do the same for indexed
> expression.  Please, register this patch on the next commitfest.

Thank you for your comment! I registered this to CF 2018-09.

> Regarding current implementation I found message "ANALYZE option must
> be specified when a column list is provided" to be confusing.
> Perhaps, you've missed some negation in this message, since in fact
> you disallow analyze with column list.
> 
> However, since multicolumn index may contain multiple expression, I
> think we should support specifying columns for ANALYZE index clause.
> We could support specifying columns by their numbers in the same way
> we did for "ALTER INDEX index ALTER COLUMN colnum SET STATISTICS
> number".

Make sense. I'll fix the patch to support specifying columns of index.

Thanks,

-- 
Yugo Nagata 

Allow to specify a index name as ANALYZE parameter

[Yugo Nagata]

Hi,

When we specify column names for ANALYZE, only the statistics for those columns
are collected. Similarly, is it useful if we have a option to specify an index
for ANALYZE to collect only the statistics for expression in the specified 
index?

A usecase I suppose is that when a new expression index is created and that
we need only the statistics for the new index. Although ANALYZE of all the 
indexes
is usually fast because ANALYZE uses a random sampling of the table rows, 
ANALYZE
on the specific index may be still useful if there are other index whose 
"statistics
target" is large and/or whose expression takes time to compute, for example.

Attached is the WIP patch to allow to specify a index name as ANALYZE parameter.
Any documatation is not yet included.  I would appreciate any feedback!

Regards,

-- 
Yugo Nagata 
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 25194e8..058f242 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -84,7 +84,7 @@ static BufferAccessStrategy vac_strategy;
 static void do_analyze_rel(Relation onerel, int options,
 			   VacuumParams *params, List *va_cols,
 			   AcquireSampleRowsFunc acquirefunc, BlockNumber relpages,
-			   bool inh, bool in_outer_xact, int elevel);
+			   bool inh, bool in_outer_xact, int elevel, Oid indexid);
 static void compute_index_stats(Relation onerel, double totalrows,
 	AnlIndexData *indexdata, int nindexes,
 	HeapTuple *rows, int numrows,
@@ -121,6 +121,7 @@ analyze_rel(Oid relid, RangeVar *relation, int options,
 	AcquireSampleRowsFunc acquirefunc = NULL;
 	BlockNumber relpages = 0;
 	bool		rel_lock = true;
+	Oid			indexid = InvalidOid;
 
 	/* Select logging level */
 	if (options & VACOPT_VERBOSE)
@@ -253,6 +254,28 @@ analyze_rel(Oid relid, RangeVar *relation, int options,
 		/* Also get regular table's size */
 		relpages = RelationGetNumberOfBlocks(onerel);
 	}
+	else if (onerel->rd_rel->relkind == RELKIND_INDEX)
+	{
+		Relation rel;
+
+		indexid = relid;
+		relid = IndexGetRelation(indexid, true);
+
+		if (va_cols != NIL)
+			ereport(ERROR,
+	(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+	 errmsg("ANALYZE option must be specified when a column list is provided")));
+
+		rel = try_relation_open(relid, ShareUpdateExclusiveLock);
+		relation_close(onerel, ShareUpdateExclusiveLock);
+
+		onerel = rel;
+
+		/* Regular table, so we'll use the regular row acquisition function */
+		acquirefunc = acquire_sample_rows;
+		/* Also get regular table's size */
+		relpages = RelationGetNumberOfBlocks(onerel);
+	}
 	else if (onerel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
 	{
 		/*
@@ -308,14 +331,14 @@ analyze_rel(Oid relid, RangeVar *relation, int options,
 	 */
 	if (onerel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
 		do_analyze_rel(onerel, options, params, va_cols, acquirefunc,
-	   relpages, false, in_outer_xact, elevel);
+	   relpages, false, in_outer_xact, elevel, indexid);
 
 	/*
 	 * If there are child tables, do recursive ANALYZE.
 	 */
 	if (onerel->rd_rel->relhassubclass)
 		do_analyze_rel(onerel, options, params, va_cols, acquirefunc, relpages,
-	   true, in_outer_xact, elevel);
+	   true, in_outer_xact, elevel, InvalidOid);
 
 	/*
 	 * Close source relation now, but keep lock so that no one deletes it
@@ -345,7 +368,7 @@ static void
 do_analyze_rel(Relation onerel, int options, VacuumParams *params,
 			   List *va_cols, AcquireSampleRowsFunc acquirefunc,
 			   BlockNumber relpages, bool inh, bool in_outer_xact,
-			   int elevel)
+			   int elevel, Oid indexid)
 {
 	int			attr_cnt,
 tcnt,
@@ -445,7 +468,7 @@ do_analyze_rel(Relation onerel, int options, VacuumParams *params,
 		}
 		attr_cnt = tcnt;
 	}
-	else
+	else if (!OidIsValid(indexid))
 	{
 		attr_cnt = onerel->rd_att->natts;
 		vacattrstats = (VacAttrStats **)
@@ -459,6 +482,8 @@ do_analyze_rel(Relation onerel, int options, VacuumParams *params,
 		}
 		attr_cnt = tcnt;
 	}
+	else
+		attr_cnt = 0;
 
 	/*
 	 * Open all indexes of the relation, and see if there are any analyzable
@@ -468,7 +493,7 @@ do_analyze_rel(Relation onerel, int options, VacuumParams *params,
 	 * all.
 	 */
 	if (!inh)
-		vac_open_indexes(onerel, AccessShareLock, , );
+		vac_open_indexes(onerel, AccessShareLock, , , indexid);
 	else
 	{
 		Irel = NULL;
@@ -750,6 +775,7 @@ compute_index_stats(Relation onerel, double totalrows,
 	int			ind,
 i;
 
+
 	ind_context = AllocSetContextCreate(anl_context,
 		"Analyze Index",
 		ALLOCSET_DEFAULT_SIZES);
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index d90cb9a..b21811d 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -1606,7 +1606,7 @@ vacuum_rel(Oid relid, RangeVar *relation, int options, VacuumParams *params)
  */
 void
 vac_open_indexes(Relation relation, LOCKMODE lockmode,
- int *nindexes, Relation 

Preferring index-only-scan when the cost is equal

[Yugo Nagata]

Hi,

I found that there is a situation that even when index only scan can be 
effective, 
the planner doesn't select this.  The planner makes indexe paths in descending
order of indexrelid, and the new path is discarded if its cost is not less than
the existing paths' cost. As a result, IndexOnlyScan path can be discard even 
hough it may be effective than normal IndexScan.

Here is a example;

=# create table test1 (i int, d int);
CREATE TABLE
=# create index on test1(i) include (d);
CREATE INDEX

=# explain select d from test1 where i = 0;
QUERY PLAN  
  
--
 Index Only Scan using test1_i_d_idx on test1  (cost=0.15..36.35 rows=11 
width=4)
   Index Cond: (i = 0)
(2 rows)

=# create index on test1(i) ;
CREATE INDEX
=# explain select d from test1 where i = 0;
QUERY PLAN 
---
 Index Scan using test1_i_idx on test1  (cost=0.15..36.35 rows=11 width=4)
   Index Cond: (i = 0)
(2 rows)


This is not new for the covered index feature. We can see the same thing when 
using
multi-column indexes.


=# create table test2 (i int, d int);
CREATE TABLE
=# create index on test2(i,d);
CREATE INDEX
=# explain select d from test2 where i = 0;
QUERY PLAN  
  
--
 Index Only Scan using test2_i_d_idx on test2  (cost=0.15..36.35 rows=11 
width=4)
   Index Cond: (i = 0)
(2 rows)

=# create index on test2(i);
CREATE INDEX
=# explain select d from test2 where i = 0;
QUERY PLAN 
---
 Index Scan using test2_i_idx on test2  (cost=0.15..36.35 rows=11 width=4)
   Index Cond: (i = 0)
(2 rows)


Attached is a patch to prefer index-only-scan when the cost is equal to other 
index
path.  Honestly, I'm not sure this is the best way. Any comments and advices 
would
be appriciated.

Regards,

-- 
Yugo Nagata 
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index dbf9adc..61f0609 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -537,6 +537,8 @@ add_path(RelOptInfo *parent_rel, Path *new_path)
 	old_path,
 	1.01) == COSTS_BETTER1)
 	remove_old = true;	/* new dominates old */
+else if (old_path->pathtype == T_IndexScan && new_path->pathtype == T_IndexOnlyScan)
+	remove_old = true;	/* new dominates old */
 else
 	accept_new = false; /* old equals or
 		 * dominates new */

Re: Typo in Japanese translation of psql.

[Yugo Nagata]

On Fri, 6 Jul 2018 08:33:56 +
Taiki Kondo  wrote:

> Hi all,
> 
> I found typo in Japanese translation of psql.

Good catch!

However, I think you have to submit the whole po file to Patch Tracker[1]
instead of a patch according to the wiki [2].

[1] https://redmine.postgresql.org/projects/pgtranslation
[2] https://wiki.postgresql.org/wiki/NLS

Regards,

> 
> Please find attached.
> 
> 
> Sincerely,
> 
> --
> Taiki Kondo
> NEC Solution Innovators, Ltd.


-- 
Yugo Nagata 

Re: Fix to not check included columns in ANALYZE on indexes

[Yugo Nagata]

On Fri, 29 Jun 2018 17:31:51 +0300
Teodor Sigaev  wrote:

> > AFAICS, we'd just have to revert this patch later, so I don't see
> > much value in it.
> True, I suppose we should apply this patch just for consistency, because we 
> don't allow expression in included columns.

Yes, this is what I intend in my patch, but I don't persist in this if there
is a reason to leave the code as it is, since the current code is alomot 
harmless.

Thanks,


-- 
Yugo Nagata 

Re: Fix to not check included columns in ANALYZE on indexes

[Yugo Nagata]

On Sat, 30 Jun 2018 14:13:49 -0400
Tom Lane  wrote:

> Peter Geoghegan  writes:
> > I think that the argument Tom is making is that it might be useful to
> > have statistics on the expression regardless of this -- the expression
> > may be interesting in some general sense. For example, one can imagine
> > the planner creating a plan with a hash aggregate rather than a group
> > aggregate, but only when statistics on an expression are available,
> > somehow.
> 
> Right.  For instance, "select sum(x) from ... group by y+z" is only
> suitable for hash aggregation if we can predict that there's a fairly
> small number of distinct values of y+z.  This makes it useful to have
> stats on the expression y+z, independently of whether any related index
> actually gets used in the plan.

Thank you for your explanation. I understand the usefulness of the statistics 
on non-key expression attributions and that "CREATE INDEX ... INCLUDE" migth be
a means to collect the statistics on "non-key" expressions in future.

> 
>   regards, tom lane
> 


-- 
Yugo Nagata 

Re: CREATE TABLE .. LIKE .. EXCLUDING documentation

[Yugo Nagata]

On Wed, 4 Jul 2018 10:46:30 +0200
Peter Eisentraut  wrote:

> On 02.07.18 10:38, Daniel Gustafsson wrote:
> >> On 29 Jun 2018, at 18:44, Tom Lane  wrote:
> > 
> >> +1 for shortening it as proposed by Peter.  The existing arrangement
> >> made sense when it was first written, when there were only about three
> >> individual options IIRC.  Now it's just confusing, especially since you
> >> can't tell very easily whether any of the individual options were
> >> intentionally omitted from the list.  It will not get better with
> >> more options, either.
> > 
> > Marking this "Waiting for Author” awaiting an update version expanding with 
> > the
> > above comment.
> 
> I ended up rewriting that whole section a bit to give it more structure.
>  I included all the points discussed in this thread.

Thank you for fixing this. 

> 
> -- 
> Peter Eisentraut  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> 


-- 
Yugo Nagata 

Re: Fix error message when trying to alter statistics on included column

[Yugo Nagata]

On Mon, 2 Jul 2018 14:23:09 -0400
Robert Haas  wrote:

> On Thu, Jun 28, 2018 at 5:28 AM, Yugo Nagata  wrote:
> > According to the error message, it is not allowed to alter statistics on
> > included column because this is "non-expression column".
> >
> >  postgres=# create table test (i int, d int);
> >  CREATE TABLE
> >  postgres=# create index idx on test(i) include (d);
> >  CREATE INDEX
> >  postgres=# alter index idx alter column 2 set statistics 10;
> >  ERROR:  cannot alter statistics on non-expression column "d" of index "idx"
> >  HINT:  Alter statistics on table column instead.
> >
> > However, I think this should be forbidded in that this is not a key column
> > but a included column. Even if we decide to support expressions in included
> > columns in future, it is meaningless to do this because any statistics on
> > included column is never used by the planner.
> >
> > Attached is the patch to fix the error message. In this fix, column number
> > is checked first. After applying this, the message is changed as below;
> >
> >  postgres=# alter index idx alter column 2 set statistics 10;
> >  ERROR:  cannot alter statistics on included column "d" of index "idx"
> 
> I think you should add an open item for this.

I was about to add this to the wiki, but someone has already done this. Thanks!
https://wiki.postgresql.org/wiki/PostgreSQL_11_Open_Items

> 
> -- 
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
> 


-- 
Yugo Nagata 

Re: CREATE TABLE .. LIKE .. EXCLUDING documentation

[Yugo Nagata]

On Fri, 29 Jun 2018 11:36:19 +0200
Daniel Gustafsson  wrote:

> > On 29 Jun 2018, at 09:14, Yugo Nagata  wrote:
> 
> > Thanks a lot.
> > 
> > I updated the patch.
> 
> This version looks good to me.  You might want to add it to the CF to make 
> sure
> it isn’t forgotten.
> 
> cheers ./daniel

Sure. Added.

-- 
Yugo Nagata 

Re: Fix to not check included columns in ANALYZE on indexes

[Yugo Nagata]

On Thu, 28 Jun 2018 19:18:36 -0400
Tom Lane  wrote:

> Yugo Nagata  writes:
> > I found that both key columns and included columns are checked when analyze 
> > is run on indexes. This is almost harmless because non-expression columns
> > are not processed. However, this check is obviously unnecessary and we
> > can fix this to not check included columns. If we decide to support 
> > expressions
> > in included columns in future, this must be fixed eventually.
> 
> AFAICS, we'd just have to revert this patch later, so I don't see
> much value in it.

I'm sorry but I don't understand why we'd just have to revert this patch later.

Do you mean that if we decide to support expressions in included columns in 
future,
this patch would be reverted? This is wrong. To my understanding, statistics on
included  (= non-key) columns in index is never used by the planner whether this
is expression or not. So, we don't have to examin these columns in ANALYZE.

> Also, is it really true that we don't support included expression
> columns now?  In what way would that not be a bug?

Currently, included expression columns are not supported.

 postgres=# create index on test(i) include ((d+1));
 ERROR:  expressions are not supported in included columns

> 
>   regards, tom lane
> 


-- 
Yugo Nagata 

Re: Forbid referencing columns by names in ALTER INDEX ... SET STATISTICS

[Yugo Nagata]

On Thu, 28 Jun 2018 10:26:13 -0400
Robert Haas  wrote:

> On Wed, Jun 27, 2018 at 9:22 AM, Yugo Nagata  wrote:
> > According to the syntax in ALTER INDEX doc, a column should be specified by
> > column number as discussed in [1]. However, the current code still allows to
> > use an internal column name like "expr". Is this intentional?
> >
> > Although it is harmless, how about forbiding this undocumented and
> > unuseful behavior. The attached patch does it.
> 
> If it's harmless, why prohibit it?

I thought that we should prohibit it because this is not allowed
according to the documented syntax. However, don't we have to be
so rigorous about it?

> 
> -- 
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
> 


-- 
Yugo Nagata 

Re: CREATE TABLE .. LIKE .. EXCLUDING documentation

[Yugo Nagata]

On Fri, 29 Jun 2018 08:39:01 +0200
Daniel Gustafsson  wrote:

> > On 29 Jun 2018, at 07:56, Yugo Nagata  wrote:
> > On Thu, 28 Jun 2018 16:22:15 -0700
> > "David G. Johnston"  wrote:
> 
> >> ​Maybe try something like:
> >> 
> >> It is legal to specify the same option multiple times - e.g., "INCLUDING
> >> option EXCLUDING option" - the outcome is whichever comes last in the
> >> command (i.e., in the example, option is excluded).
> > 
> > Certainly. However, it seems to me that example is also redundant.
> > I rewrote this as follows:
> > 
> > It is legal to specify multiple options for the same kind of object. 
> > If they conflict, latter options always override former options. 
> > 
> > Does this make sense?
> 
> I think this wording makes sense and is clear. Only found a small typo:
> 
> +      This is tipically used after INCLUDING ALL.  
> 
> s/tipically/typically/

Thanks a lot.

I updated the patch.

> 
> cheers ./daniel


-- 
Yugo Nagata 
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 2a1eac9..6147d86 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -624,6 +624,13 @@ WITH ( MODULUS numeric_literal, REM
   INCLUDING COMMENTS INCLUDING CONSTRAINTS INCLUDING DEFAULTS INCLUDING IDENTITY INCLUDING INDEXES INCLUDING STATISTICS INCLUDING STORAGE.
  
  
+  If EXCLUDING option is specified after
+  INCLUDING option, the referenced objects are not copied.
+  This is typically used after INCLUDING ALL.  
+  It is legal to specify multiple options for the same kind of object.
+  If they conflict, latter options always override former options.
+ 
+ 
   Note that unlike INHERITS, columns and
   constraints copied by LIKE are not merged with similarly
   named columns and constraints.

Re: CREATE TABLE .. LIKE .. EXCLUDING documentation

[Yugo Nagata]

On Thu, 28 Jun 2018 16:22:15 -0700
"David G. Johnston"  wrote:

> On Thu, Jun 28, 2018 at 3:57 PM, Daniel Gustafsson  wrote:

Thank you for your reviewing!

I attached the updated patch.

> 
> > > On 27 Jun 2018, at 18:02, Yugo Nagata  wrote:
> >
> > > I found that there isn't explanation about EXCLUDING in CREATE TABLE doc.
> > > Attached is a patch to add this. I would appreciate it if a native
> > English
> > > speaker comments on this.
> >
> > +  If EXCLUDING option  is
> > specified
> >
> > The empty  seems wrong.

Fixed

> >
> > +  after INCLUDING options, the specified thing is
> > excluded
> >
> > “thing” sounds a bit vague here (and in the last sentence as well), but I’m
> > also not sure what to use instead.  “referenced objects" perhaps?

Fixed.

> >
> > +1 on documenting the EXCLUDING option though.
> >
> 
> ​"is excluded" and "not copied" are redundant to each other and the first

I removed "is excluded".

> sentence is basically redundant with the second.
> 
> ​Maybe try something like:
> 
> It is legal to specify the same option multiple times - e.g., "INCLUDING
> option EXCLUDING option" - the outcome is whichever comes last in the
> command (i.e., in the example, option is excluded).

Certainly. However, it seems to me that example is also redundant.
I rewrote this as follows:

 It is legal to specify multiple options for the same kind of object. 
 If they conflict, latter options always override former options. 

Does this make sense?

> 
> David J.


-- 
Yugo Nagata 
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 2a1eac9..d57af73 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -624,6 +624,13 @@ WITH ( MODULUS numeric_literal, REM
   INCLUDING COMMENTS INCLUDING CONSTRAINTS INCLUDING DEFAULTS INCLUDING IDENTITY INCLUDING INDEXES INCLUDING STATISTICS INCLUDING STORAGE.
  
  
+  If EXCLUDING option is specified after
+  INCLUDING option, the referenced objects are not copied.
+  This is tipically used after INCLUDING ALL.  
+  It is legal to specify multiple options for the same kind of object.
+  If they conflict, latter options always override former options.
+ 
+ 
   Note that unlike INHERITS, columns and
   constraints copied by LIKE are not merged with similarly
   named columns and constraints.

Fix to not check included columns in ANALYZE on indexes

[Yugo Nagata]

Hi,

I found that both key columns and included columns are checked when analyze 
is run on indexes. This is almost harmless because non-expression columns
are not processed. However, this check is obviously unnecessary and we
can fix this to not check included columns. If we decide to support expressions
in included columns in future, this must be fixed eventually.

Attached is a patch to fix this.

Regards,

-- 
Yugo Nagata 
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 25194e8..d2b2c39 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -493,7 +493,7 @@ do_analyze_rel(Relation onerel, int options, VacuumParams *params,
 thisdata->vacattrstats = (VacAttrStats **)
 	palloc(indexInfo->ii_NumIndexAttrs * sizeof(VacAttrStats *));
 tcnt = 0;
-for (i = 0; i < indexInfo->ii_NumIndexAttrs; i++)
+for (i = 0; i < indexInfo->ii_NumIndexKeyAttrs; i++)
 {
 	int			keycol = indexInfo->ii_IndexAttrNumbers[i];
 

Fix error message when trying to alter statistics on included column

[Yugo Nagata]

Hi,

According to the error message, it is not allowed to alter statistics on
included column because this is "non-expression column".

 postgres=# create table test (i int, d int);
 CREATE TABLE
 postgres=# create index idx on test(i) include (d);
 CREATE INDEX
 postgres=# alter index idx alter column 2 set statistics 10;
 ERROR:  cannot alter statistics on non-expression column "d" of index "idx"
 HINT:  Alter statistics on table column instead.

However, I think this should be forbidded in that this is not a key column 
but a included column. Even if we decide to support expressions in included
columns in future, it is meaningless to do this because any statistics on 
included column is never used by the planner.

Attached is the patch to fix the error message. In this fix, column number
is checked first. After applying this, the message is changed as below;

 postgres=# alter index idx alter column 2 set statistics 10;
 ERROR:  cannot alter statistics on included column "d" of index "idx"

Regards,

-- 
Yugo Nagata 
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7c0cf0d..4beb160 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6504,14 +6504,21 @@ ATExecSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newVa
  errmsg("cannot alter system column \"%s\"",
 		colName)));
 
-	if ((rel->rd_rel->relkind == RELKIND_INDEX ||
-		 rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX) &&
-		rel->rd_index->indkey.values[attnum - 1] != 0)
-		ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot alter statistics on non-expression column \"%s\" of index \"%s\"",
-		NameStr(attrtuple->attname), RelationGetRelationName(rel)),
- errhint("Alter statistics on table column instead.")));
+	if (rel->rd_rel->relkind == RELKIND_INDEX ||
+		 rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
+	{
+		if (attnum > rel->rd_index->indnkeyatts)
+			ereport(ERROR,
+	(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+	 errmsg("cannot alter statistics on included column \"%s\" of index \"%s\"",
+			NameStr(attrtuple->attname), RelationGetRelationName(rel;
+		else if (rel->rd_index->indkey.values[attnum - 1] != 0)
+			ereport(ERROR,
+	(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+	 errmsg("cannot alter statistics on non-expression column \"%s\" of index \"%s\"",
+			NameStr(attrtuple->attname), RelationGetRelationName(rel)),
+	 errhint("Alter statistics on table column instead.")));
+	}
 
 	attrtuple->attstattarget = newtarget;
 

Re: Small fixes about backup history file in doc and pg_standby

[Yugo Nagata]

On Wed, 27 Jun 2018 18:36:46 +0900
Michael Paquier  wrote:

> On Wed, Jun 27, 2018 at 05:42:07PM +0900, Yugo Nagata wrote:
> > On Wed, 27 Jun 2018 00:58:18 +0900
> > Fujii Masao  wrote:
> >>> In addition, the current pg_standby still can handle a backup history 
> >>> file that are
> >>> never requested. It is harmless but unnecessary code. Another attached 
> >>> patch
> >>> (pg_standby.patch) removes this part of code.
> >> 
> >> Since this is not bug fix, let's discuss this in 12dev cycle.
> 
> +1.

I added this to CF.

> --
> Michael


-- 
Yugo Nagata 

CREATE TABLE .. LIKE .. EXCLUDING documentation

[Yugo Nagata]

Hi,

I found that there isn't explanation about EXCLUDING in CREATE TABLE doc.
Attached is a patch to add this. I would appreciate it if a native English
speaker comments on this.

Regards,

-- 
Yugo Nagata 
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 2a1eac9..91dc006 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -624,6 +624,13 @@ WITH ( MODULUS numeric_literal, REM
   INCLUDING COMMENTS INCLUDING CONSTRAINTS INCLUDING DEFAULTS INCLUDING IDENTITY INCLUDING INDEXES INCLUDING STATISTICS INCLUDING STORAGE.
  
  
+  If EXCLUDING option  is specified
+  after INCLUDING options, the specified thing is excluded
+  and not copied.  Note that latter options always override former options when
+  they conflict, so if INCLUDING option is specified after
+  EXCLUDING option, specified things will be copied.
+ 
+ 
   Note that unlike INHERITS, columns and
   constraints copied by LIKE are not merged with similarly
   named columns and constraints.

Forbid referencing columns by names in ALTER INDEX ... SET STATISTICS

[Yugo Nagata]

Hi,

According to the syntax in ALTER INDEX doc, a column should be specified by
column number as discussed in [1]. However, the current code still allows to
use an internal column name like "expr". Is this intentional?

Although it is harmless, how about forbiding this undocumented and
unuseful behavior. The attached patch does it.

[1] 
https://www.postgresql.org/message-id/CAPpHfdsSYo6xpt0F%3DngAdqMPFJJhC7zApde9h1qwkdpHpwFisA%40mail.gmail.com

-- 
Yugo Nagata 
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7c0cf0d..1b6f278 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6430,6 +6430,12 @@ ATPrepSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newVa
 		ereport(ERROR,
 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
  errmsg("cannot refer to non-index column by number")));
+	else if ((rel->rd_rel->relkind == RELKIND_INDEX ||
+			  rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX) &&
+			  colName)
+		ereport(ERROR,
+(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot refer to expression index column by name")));
 
 	/* Permissions checks */
 	if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId()))

Re: Small fixes about backup history file in doc and pg_standby

[Yugo Nagata]

On Tue, 26 Jun 2018 20:19:42 +0900 (Tokyo Standard Time)
Kyotaro HORIGUCHI  wrote:

> Hello.
> 
> Good catch!
> 
> At Tue, 26 Jun 2018 17:47:52 +0900, Yugo Nagata  wrote 
> in <20180626174752.0ce505e3.nag...@sraoss.co.jp>
> > Hi,
> > 
> > While looking into the backup and recovery code, I found small 
> > documentation bugs. 
> > The documatation says that the backup history files can be requested for 
> > recovery, 
> > but it's not used by the system and not requested anymore since PG 9.0 
> > (commit 06f82b29616cd9effcaefd99c6b6e2e80697482f) and never be requested.
> > 
> > Attached patch (doc_backup_history_file.patch) corrects the description 
> > about this.
> > 
> > In addition, the current pg_standby still can handle a backup history file 
> > that are
> > never requested. It is harmless but unnecessary code. Another attached patch
> > (pg_standby.patch) removes this part of code.
> 
> The comment fix seems fine and they seem to be all occurances of
> the word ".backup" in the context of recovery_command.
> 
> The definition of the symbol XLOG_BACKUP_LABEL is no longer
> useful after your patch applied. Removing the symbol makes
> XLOG_DATA and the variable nextWALFileName useless and finally we
> can remove all branching using it.

Thank you for your reviewing my patch.

I've also removed XLOG_BACKUP_LABEL, but I left nextWALFileName
since this is still referred in CustomizableCleanupPriorWALFiles().

Attached is the updated patch.

Regards,

> 
> regards.
> 
> -- 
> Kyotaro Horiguchi
> NTT Open Source Software Center
> 
> 


-- 
Yugo Nagata 
diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c
index cb78597..b37cf6d 100644
--- a/contrib/pg_standby/pg_standby.c
+++ b/contrib/pg_standby/pg_standby.c
@@ -94,7 +94,6 @@ int			restoreCommandType;
 
 #define XLOG_DATA			 0
 #define XLOG_HISTORY		 1
-#define XLOG_BACKUP_LABEL	 2
 int			nextWALFileType;
 
 #define SET_RESTORE_COMMAND(cmd, arg1, arg2) \
@@ -211,15 +210,9 @@ CustomizableNextWALFileReady(void)
 		}
 
 		/*
-		 * If it's a backup file, return immediately. If it's a regular file
 		 * return only if it's the right size already.
 		 */
-		if (IsBackupHistoryFileName(nextWALFileName))
-		{
-			nextWALFileType = XLOG_BACKUP_LABEL;
-			return true;
-		}
-		else if (WalSegSz > 0 && stat_buf.st_size == WalSegSz)
+		if (WalSegSz > 0 && stat_buf.st_size == WalSegSz)
 		{
 #ifdef WIN32
 

Re: Small fixes about backup history file in doc and pg_standby

[Yugo Nagata]

On Wed, 27 Jun 2018 00:58:18 +0900
Fujii Masao  wrote:

> On Tue, Jun 26, 2018 at 5:47 PM, Yugo Nagata  wrote:
> > Hi,
> >
> > While looking into the backup and recovery code, I found small 
> > documentation bugs.
> > The documatation says that the backup history files can be requested for 
> > recovery,
> > but it's not used by the system and not requested anymore since PG 9.0
> > (commit 06f82b29616cd9effcaefd99c6b6e2e80697482f) and never be requested.
> >
> > Attached patch (doc_backup_history_file.patch) corrects the description 
> > about this.
> 
> Pushed. Thanks!

Thanks!

> 
> > In addition, the current pg_standby still can handle a backup history file 
> > that are
> > never requested. It is harmless but unnecessary code. Another attached patch
> > (pg_standby.patch) removes this part of code.
> 
> Since this is not bug fix, let's discuss this in 12dev cycle.

Certainly.

Regards,

> 
> Regards,
> 
> -- 
> Fujii Masao
> 


-- 
Yugo Nagata 

Small fixes about backup history file in doc and pg_standby

[Yugo Nagata]

Hi,

While looking into the backup and recovery code, I found small documentation 
bugs. 
The documatation says that the backup history files can be requested for 
recovery, 
but it's not used by the system and not requested anymore since PG 9.0 
(commit 06f82b29616cd9effcaefd99c6b6e2e80697482f) and never be requested.

Attached patch (doc_backup_history_file.patch) corrects the description about 
this.

In addition, the current pg_standby still can handle a backup history file that 
are
never requested. It is harmless but unnecessary code. Another attached patch
(pg_standby.patch) removes this part of code.

Regards,

-- 
Yugo Nagata 
diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c
index cb78597..d957f44 100644
--- a/contrib/pg_standby/pg_standby.c
+++ b/contrib/pg_standby/pg_standby.c
@@ -211,15 +211,9 @@ CustomizableNextWALFileReady(void)
 		}
 
 		/*
-		 * If it's a backup file, return immediately. If it's a regular file
 		 * return only if it's the right size already.
 		 */
-		if (IsBackupHistoryFileName(nextWALFileName))
-		{
-			nextWALFileType = XLOG_BACKUP_LABEL;
-			return true;
-		}
-		else if (WalSegSz > 0 && stat_buf.st_size == WalSegSz)
+		if (WalSegSz > 0 && stat_buf.st_size == WalSegSz)
 		{
 #ifdef WIN32
 
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 982776c..3fa5efd 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -1288,7 +1288,7 @@ restore_command = 'cp /mnt/server/archivedir/%f %p'

 Not all of the requested files will be WAL segment
 files; you should also expect requests for files with a suffix of
-.backup or .history. Also be aware that
+.history. Also be aware that
 the base name of the %p path will be different from
 %f; do not expect them to be interchangeable.

diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index 46bf198..934eb90 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -1524,7 +1524,7 @@ synchronous_standby_names = 'ANY 2 (s1, s2, s3)'
 processing would request a file from the WAL archive, reporting failure
 if the file was unavailable.  For standby processing it is normal for
 the next WAL file to be unavailable, so the standby must wait for
-it to appear. For files ending in .backup or
+it to appear. For files ending in 
 .history there is no need to wait, and a non-zero return
 code must be returned. A waiting restore_command can be
 written as a custom script that loops after polling for the existence of

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

On Thu, 05 Apr 2018 07:53:42 +0900 (JST)
Tatsuo Ishii <is...@sraoss.co.jp> wrote:

I update the patch to fix the lockable view issues.

> >> > > +typedef struct
> >> > > +{
> >> > > +  Oid root_reloid;
> >> > > +  LOCKMODE lockmode;
> >> > > +  bool nowait;
> >> > > +  Oid viewowner;
> >> > > +  Oid viewoid;
> >> > > +} LockViewRecurse_context;
> >> > 
> >> > Probably wouldn't hurt to pgindent the larger changes in the patch.
> 
> Yeah. Also, each struct member needs a comment.

I applied pgindent and added comments to struct members.

> 
> >> > Hm, how can that happen? And if it can happen, why can it only happen
> >> > with the root relation?
> >> 
> >> For example, the following queries cause the infinite recursion of views. 
> >> This is detected and the error is raised.
> >> 
> >>  create table t (i int);
> >>  create view v1 as select 1;
> >>  create view v2 as select * from v1;
> >>  create or replace view v1 as select * from v2;
> >>  begin;
> >>  lock v1;
> >>  abort;
> >> 
> >> However, I found that the previous patch could not handle the following
> >> situation in which the root relation itself doesn't have infinite 
> >> recursion.
> >> 
> >>  create view v3 as select * from v1;
> >>  begin;
> >>  lock v3;
> >>  abort;
> 
> Shouldn't they be in the regression test?

Added tests for the infinite recursion detection.

Regards,

> 
> It's shame that create_view test does not include the cases, but it's
> another story.
> 
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp


-- 
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
index 96d477c..a225cea 100644
--- a/doc/src/sgml/ref/lock.sgml
+++ b/doc/src/sgml/ref/lock.sgml
@@ -46,8 +46,8 @@ LOCK [ TABLE ] [ ONLY ] name [ * ]
   
 
   
-   When a view is specified to be locked, all relations appearing in the view
-   definition query are also locked recursively with the same lock mode. 
+   When a view is locked, all relations appearing in the view definition
+   query are also locked recursively with the same lock mode.
   
 
   
@@ -174,6 +174,13 @@ LOCK [ TABLE ] [ ONLY ] name [ * ]

 

+The user performing the lock on the view must have the corresponding privilege
+on the view.  In addition the view's owner must have the relevant privileges on
+the underlying base relations, but the user performing the lock does
+not need any permissions on the underlying base relations.
+   
+
+   
 LOCK TABLE is useless outside a transaction block: the lock
 would remain held only to the completion of the statement.  Therefore
 PostgreSQL reports an error if LOCK
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index b247c0f..5e0ef48 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -31,7 +31,7 @@ static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid use
 static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode, Oid userid);
 static void RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid,
 			 Oid oldrelid, void *arg);
-static void LockViewRecurse(Oid reloid, Oid root_reloid, LOCKMODE lockmode, bool nowait);
+static void LockViewRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, List *ancestor_views);
 
 /*
  * LOCK TABLE
@@ -67,7 +67,7 @@ LockTableCommand(LockStmt *lockstmt)
 		  (void *) >mode);
 
 		if (get_rel_relkind(reloid) == RELKIND_VIEW)
-			LockViewRecurse(reloid, reloid, lockstmt->mode, lockstmt->nowait);
+			LockViewRecurse(reloid, lockstmt->mode, lockstmt->nowait, NIL);
 		else if (recurse)
 			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId());
 	}
@@ -92,7 +92,6 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
 		return;	/* woops, concurrently dropped; no permissions
  * check */
 
-
 	/* Currently, we only allow plain tables or views to be locked */
 	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE &&
 		relkind != RELKIND_VIEW)
@@ -178,11 +177,11 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid)
 
 typedef struct
 {
-	Oid root_reloid;
-	LOCKMODE lockmode;
-	bool nowait;
-	Oid viewowner;
-	Oid viewoid;
+	LOCKMODE	lockmode;		/* lock mode to use */
+	bool		nowait;			/* no wait mode */
+	Oid			viewowner;		/* view owner for checking the privilege */
+	Oid			viewoid;		/* OID of the view to be

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

On Mon, 2 Apr 2018 18:32:53 +0900
Yugo Nagata <nag...@sraoss.co.jp> wrote:

> On Thu, 29 Mar 2018 17:26:36 -0700
> Andres Freund <and...@anarazel.de> wrote:
> 
> Thank you for your comments. I attach a patch to fix issues
> you've pointed out.

I found a typo in the documentation and attach the updated patch.

Regards,

> 
> > Hi,
> > 
> > On 2018-03-28 20:26:48 +0900, Yugo Nagata wrote:
> > > diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
> > > index b2c7203..96d477c 100644
> > > --- a/doc/src/sgml/ref/lock.sgml
> > > +++ b/doc/src/sgml/ref/lock.sgml
> > > @@ -46,6 +46,11 @@ LOCK [ TABLE ] [ ONLY ]  > > class="parameter">name [ * ]
> > >
> > >  
> > >
> > > +   When a view is specified to be locked, all relations appearing in the 
> > > view
> > > +   definition query are also locked recursively with the same lock mode. 
> > > +  
> > 
> > Trailing space added. I'd remove "specified to be" from the sentence.
> 
> Fixed.
> 
> > 
> > I think mentioning how this interacts with permissions would be a good
> > idea too. Given how operations use the view's owner to recurse, that's
> > not obvious. Should also explain what permissions are required to do the
> > operation on the view.
> 
> Added a description about permissions into the documentation.
> 
> > 
> > 
> > > @@ -86,15 +92,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid 
> > > relid, Oid oldrelid,
> > >   return; /* woops, concurrently 
> > > dropped; no permissions
> > >* check */
> > >  
> > > - /* Currently, we only allow plain tables to be locked */
> > > - if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
> > > +
> > 
> > This newline looks spurious to me.
> 
> Removed.
> 
> > 
> > 
> > >  /*
> > > + * Apply LOCK TABLE recursively over a view
> > > + *
> > > + * All tables and views appearing in the view definition query are locked
> > > + * recursively with the same lock mode.
> > > + */
> > > +
> > > +typedef struct
> > > +{
> > > + Oid root_reloid;
> > > + LOCKMODE lockmode;
> > > + bool nowait;
> > > + Oid viewowner;
> > > + Oid viewoid;
> > > +} LockViewRecurse_context;
> > 
> > Probably wouldn't hurt to pgindent the larger changes in the patch.
> > 
> > 
> > > +static bool
> > > +LockViewRecurse_walker(Node *node, LockViewRecurse_context *context)
> > > +{
> > > + if (node == NULL)
> > > + return false;
> > > +
> > > + if (IsA(node, Query))
> > > + {
> > > + Query   *query = (Query *) node;
> > > + ListCell*rtable;
> > > +
> > > + foreach(rtable, query->rtable)
> > > + {
> > > + RangeTblEntry   *rte = lfirst(rtable);
> > > + AclResultaclresult;
> > > +
> > > + Oid relid = rte->relid;
> > > + char relkind = rte->relkind;
> > > + char *relname = get_rel_name(relid);
> > > +
> > > + /* The OLD and NEW placeholder entries in the view's 
> > > rtable are skipped. */
> > > + if (relid == context->viewoid &&
> > > + (!strcmp(rte->eref->aliasname, "old") || 
> > > !strcmp(rte->eref->aliasname, "new")))
> > > + continue;
> > > +
> > > + /* Currently, we only allow plain tables or views to be 
> > > locked. */
> > > + if (relkind != RELKIND_RELATION && relkind != 
> > > RELKIND_PARTITIONED_TABLE &&
> > > + relkind != RELKIND_VIEW)
> > > + continue;
> > > +
> > > + /* Check infinite recursion in the view definition. */
> > > + if (relid == context->root_reloid)
> > > + ereport(ERROR,
> > > + 
> > > (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> > > +     errmsg("infinite recursion 
> > > d

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

On Thu, 29 Mar 2018 17:26:36 -0700
Andres Freund <and...@anarazel.de> wrote:

Thank you for your comments. I attach a patch to fix issues
you've pointed out.

> Hi,
> 
> On 2018-03-28 20:26:48 +0900, Yugo Nagata wrote:
> > diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
> > index b2c7203..96d477c 100644
> > --- a/doc/src/sgml/ref/lock.sgml
> > +++ b/doc/src/sgml/ref/lock.sgml
> > @@ -46,6 +46,11 @@ LOCK [ TABLE ] [ ONLY ]  > class="parameter">name [ * ]
> >
> >  
> >
> > +   When a view is specified to be locked, all relations appearing in the 
> > view
> > +   definition query are also locked recursively with the same lock mode. 
> > +  
> 
> Trailing space added. I'd remove "specified to be" from the sentence.

Fixed.

> 
> I think mentioning how this interacts with permissions would be a good
> idea too. Given how operations use the view's owner to recurse, that's
> not obvious. Should also explain what permissions are required to do the
> operation on the view.

Added a description about permissions into the documentation.

> 
> 
> > @@ -86,15 +92,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid 
> > relid, Oid oldrelid,
> > return; /* woops, concurrently 
> > dropped; no permissions
> >  * check */
> >  
> > -   /* Currently, we only allow plain tables to be locked */
> > -   if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
> > +
> 
> This newline looks spurious to me.

Removed.

> 
> 
> >  /*
> > + * Apply LOCK TABLE recursively over a view
> > + *
> > + * All tables and views appearing in the view definition query are locked
> > + * recursively with the same lock mode.
> > + */
> > +
> > +typedef struct
> > +{
> > +   Oid root_reloid;
> > +   LOCKMODE lockmode;
> > +   bool nowait;
> > +   Oid viewowner;
> > +   Oid viewoid;
> > +} LockViewRecurse_context;
> 
> Probably wouldn't hurt to pgindent the larger changes in the patch.
> 
> 
> > +static bool
> > +LockViewRecurse_walker(Node *node, LockViewRecurse_context *context)
> > +{
> > +   if (node == NULL)
> > +   return false;
> > +
> > +   if (IsA(node, Query))
> > +   {
> > +   Query   *query = (Query *) node;
> > +   ListCell*rtable;
> > +
> > +   foreach(rtable, query->rtable)
> > +   {
> > +   RangeTblEntry   *rte = lfirst(rtable);
> > +   AclResultaclresult;
> > +
> > +   Oid relid = rte->relid;
> > +   char relkind = rte->relkind;
> > +   char *relname = get_rel_name(relid);
> > +
> > +   /* The OLD and NEW placeholder entries in the view's 
> > rtable are skipped. */
> > +   if (relid == context->viewoid &&
> > +   (!strcmp(rte->eref->aliasname, "old") || 
> > !strcmp(rte->eref->aliasname, "new")))
> > +   continue;
> > +
> > +   /* Currently, we only allow plain tables or views to be 
> > locked. */
> > +   if (relkind != RELKIND_RELATION && relkind != 
> > RELKIND_PARTITIONED_TABLE &&
> > +   relkind != RELKIND_VIEW)
> > +   continue;
> > +
> > +   /* Check infinite recursion in the view definition. */
> > +   if (relid == context->root_reloid)
> > +   ereport(ERROR,
> > +   
> > (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> > +   errmsg("infinite recursion 
> > detected in rules for relation \"%s\"",
> > +   
> > get_rel_name(context->root_reloid;
> 
> Hm, how can that happen? And if it can happen, why can it only happen
> with the root relation?

For example, the following queries cause the infinite recursion of views. 
This is detected and the error is raised.

 create table t (i int);
 create view v1 as select 1;
 create view v2 as select * from v1;
 create or replace view v1 as select * from v2;
 begin;
 lock v1;
 abort;

However, I found that the previous patch could not handle the following
situati

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

On Wed, 28 Mar 2018 15:45:09 +0900 (JST)
Tatsuo Ishii <is...@sraoss.co.jp> wrote:

> >> I found the previous patch was broken and this can't handle
> >> views that has subqueries as bellow;
> >> 
> >> 　CREATE VIEW lock_view6 AS SELECT * from (select * from lock_tbl1) sub;
> >> 
> >> I fixed this and attached the updated version including additional tests.
> > 
> > This patch gives a warning while compiling:
> > 
> > lockcmds.c:186:1: warning: no semicolon at end of struct or union
> >  } LockViewRecurse_context;
> >  ^
> 
> Also I get a regression test failure:

Thank you for your reviewing my patch.
I attached the updated patch, v10.

Regards,

> 
> *** 
> /usr/local/src/pgsql/current/postgresql/src/test/regress/expected/lock.out
> 2018-03-28 15:24:13.805314577 +0900
> --- /usr/local/src/pgsql/current/postgresql/src/test/regress/results/lock.out 
> 2018-03-28 15:42:07.975592594 +0900
> ***
> *** 118,124 
>   
>lock_tbl1
>lock_view6
> ! (2 rows)
>   
>   ROLLBACK;
>   -- Verify that we can lock a table with inheritance children.
> --- 118,125 
>   
>lock_tbl1
>lock_view6
> !  mvtest_tm
> ! (3 rows)
> 
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp


-- 
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
index b2c7203..96d477c 100644
--- a/doc/src/sgml/ref/lock.sgml
+++ b/doc/src/sgml/ref/lock.sgml
@@ -46,6 +46,11 @@ LOCK [ TABLE ] [ ONLY ] name [ * ]
   
 
   
+   When a view is specified to be locked, all relations appearing in the view
+   definition query are also locked recursively with the same lock mode. 
+  
+
+  
When acquiring locks automatically for commands that reference
tables, PostgreSQL always uses the least
restrictive lock mode possible. LOCK TABLE
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 6479dcb..e8a8877 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -23,11 +23,15 @@
 #include "utils/acl.h"
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
+#include "rewrite/rewriteHandler.h"
+#include "access/heapam.h"
+#include "nodes/nodeFuncs.h"
 
-static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait);
-static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode);
+static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid);
+static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode, Oid userid);
 static void RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid,
 			 Oid oldrelid, void *arg);
+static void LockViewRecurse(Oid reloid, Oid root_reloid, LOCKMODE lockmode, bool nowait);
 
 /*
  * LOCK TABLE
@@ -62,8 +66,10 @@ LockTableCommand(LockStmt *lockstmt)
 		  RangeVarCallbackForLockTable,
 		  (void *) >mode);
 
-		if (recurse)
-			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait);
+		if (get_rel_relkind(reloid) == RELKIND_VIEW)
+			LockViewRecurse(reloid, reloid, lockstmt->mode, lockstmt->nowait);
+		else if (recurse)
+			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId());
 	}
 }
 
@@ -86,15 +92,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
 		return;	/* woops, concurrently dropped; no permissions
  * check */
 
-	/* Currently, we only allow plain tables to be locked */
-	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
+
+	/* Currently, we only allow plain tables or views to be locked */
+	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE &&
+		relkind != RELKIND_VIEW)
 		ereport(ERROR,
 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table",
+ errmsg("\"%s\" is not a table or view",
 		rv->relname)));
 
 	/* Check permissions. */
-	aclresult = LockTableAclCheck(relid, lockmode);
+	aclresult = LockTableAclCheck(relid, lockmode, GetUserId());
 	if (aclresult != ACLCHECK_OK)
 		aclcheck_error(aclresult, get_relkind_objtype(get_rel_relkind(relid)), rv->relname);
 }
@@ -107,7 +115,7 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
  * multiply-inheriting children more than once, but that's no problem.
  */
 static void
-LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait)
+LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid)
 {
 	List	   *children;
 	ListCell   *lc;
@@ -120,7 +128,7 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait)
 		AclResult	aclresult

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

On Tue, 27 Mar 2018 23:28:04 +0900
Yugo Nagata <nag...@sraoss.co.jp> wrote:

I found the previous patch was broken and this can't handle
views that has subqueries as bellow;

　CREATE VIEW lock_view6 AS SELECT * from (select * from lock_tbl1) sub;

I fixed this and attached the updated version including additional tests.

Regards,

> On Tue, 6 Feb 2018 11:12:37 -0500
> Robert Haas <robertmh...@gmail.com> wrote:
> 
> > On Tue, Feb 6, 2018 at 1:28 AM, Tatsuo Ishii <is...@sraoss.co.jp> wrote:
> > >> But what does that have to do with locking?
> > >
> > > Well, if the view is not updatable, I think there will be less point
> > > to allow to lock the base tables in the view because locking is
> > > typically used in a case when updates are required.
> > >
> > > Of course we could add special triggers to allow to update views that
> > > are not automatically updatable but that kind of views are tend to
> > > complex and IMO there's less need the automatic view locking feature.
> > 
> > Hmm.  Well, I see now why you've designed the feature in the way that
> > you have, but I guess it still seems somewhat arbitrary to me.  If you
> > ignore the deadlock consideration, then there's no reason not to
> > define the feature as locking every table mentioned anywhere in the
> > query, including subqueries, and it can work for all views whether
> > updatable or not.  If the deadlock consideration is controlling, then
> > I guess we can't do better than what you have, but I'm not sure how
> > future-proof it is.  If in the future somebody makes views updateable
> > that involve a join, say from the primary key of one table to a unique
> > key of another so that no duplicate rows can be introduced, then
> > they'll either have to write code to make this feature identify and
> > lock the "main" table, which I'm not sure would be strong enough in
> > all cases, or lock them all, which reintroduces the deadlock problem.
> > 
> > Personally, I would be inclined to view the deadlock problem as not
> > very important.  I just don't see how that is going to come up very
> 
> I agree that the deadlock won't occur very often and this is not
> so important.
> 
> I have updated the lockable-view patch to v8.
> 
> This new version doen't consider the deadlock problem, and all tables
> or views appearing in the view definition query are locked recursively.
> Also, this allows all kinds of views to be locked even if it is not
> auto-updatable view.
> 
> 
> > often.  What I do think will be an issue is that if you start locking
> > lots of tables, you might prevent the system from getting much work
> > done, whether or not you also cause any deadlocks.  But I don't see
> > what we can do about that, really.  If users want full control over
> > which tables get locked, then they have to name them explicitly.  Or
> > alternatively, maybe they should avoid the need for full-table locks
> > by using SSI, gaining the benefits of (1) optimistic rather than
> > pessimistic concurrency control, (2) finer-grained locking, and (3)
> > not needing to issue explicit LOCK commands.
> 
> 
> 
> > -- 
> > Robert Haas
> > EnterpriseDB: http://www.enterprisedb.com
> > The Enterprise PostgreSQL Company
> 
> 
> -- 
> Yugo Nagata <nag...@sraoss.co.jp>


-- 
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
index b2c7203..96d477c 100644
--- a/doc/src/sgml/ref/lock.sgml
+++ b/doc/src/sgml/ref/lock.sgml
@@ -46,6 +46,11 @@ LOCK [ TABLE ] [ ONLY ] name [ * ]
   
 
   
+   When a view is specified to be locked, all relations appearing in the view
+   definition query are also locked recursively with the same lock mode. 
+  
+
+  
When acquiring locks automatically for commands that reference
tables, PostgreSQL always uses the least
restrictive lock mode possible. LOCK TABLE
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 6479dcb..e15d87d 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -23,11 +23,15 @@
 #include "utils/acl.h"
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
+#include "rewrite/rewriteHandler.h"
+#include "access/heapam.h"
+#include "nodes/nodeFuncs.h"
 
-static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait);
-static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode);
+static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid);
+static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode, Oid userid);
 static void Rang

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

On Tue, 6 Feb 2018 11:12:37 -0500
Robert Haas <robertmh...@gmail.com> wrote:

> On Tue, Feb 6, 2018 at 1:28 AM, Tatsuo Ishii <is...@sraoss.co.jp> wrote:
> >> But what does that have to do with locking?
> >
> > Well, if the view is not updatable, I think there will be less point
> > to allow to lock the base tables in the view because locking is
> > typically used in a case when updates are required.
> >
> > Of course we could add special triggers to allow to update views that
> > are not automatically updatable but that kind of views are tend to
> > complex and IMO there's less need the automatic view locking feature.
> 
> Hmm.  Well, I see now why you've designed the feature in the way that
> you have, but I guess it still seems somewhat arbitrary to me.  If you
> ignore the deadlock consideration, then there's no reason not to
> define the feature as locking every table mentioned anywhere in the
> query, including subqueries, and it can work for all views whether
> updatable or not.  If the deadlock consideration is controlling, then
> I guess we can't do better than what you have, but I'm not sure how
> future-proof it is.  If in the future somebody makes views updateable
> that involve a join, say from the primary key of one table to a unique
> key of another so that no duplicate rows can be introduced, then
> they'll either have to write code to make this feature identify and
> lock the "main" table, which I'm not sure would be strong enough in
> all cases, or lock them all, which reintroduces the deadlock problem.
> 
> Personally, I would be inclined to view the deadlock problem as not
> very important.  I just don't see how that is going to come up very

I agree that the deadlock won't occur very often and this is not
so important.

I have updated the lockable-view patch to v8.

This new version doen't consider the deadlock problem, and all tables
or views appearing in the view definition query are locked recursively.
Also, this allows all kinds of views to be locked even if it is not
auto-updatable view.


> often.  What I do think will be an issue is that if you start locking
> lots of tables, you might prevent the system from getting much work
> done, whether or not you also cause any deadlocks.  But I don't see
> what we can do about that, really.  If users want full control over
> which tables get locked, then they have to name them explicitly.  Or
> alternatively, maybe they should avoid the need for full-table locks
> by using SSI, gaining the benefits of (1) optimistic rather than
> pessimistic concurrency control, (2) finer-grained locking, and (3)
> not needing to issue explicit LOCK commands.



> -- 
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company


-- 
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
index b2c7203..96d477c 100644
--- a/doc/src/sgml/ref/lock.sgml
+++ b/doc/src/sgml/ref/lock.sgml
@@ -46,6 +46,11 @@ LOCK [ TABLE ] [ ONLY ] name [ * ]
   
 
   
+   When a view is specified to be locked, all relations appearing in the view
+   definition query are also locked recursively with the same lock mode. 
+  
+
+  
When acquiring locks automatically for commands that reference
tables, PostgreSQL always uses the least
restrictive lock mode possible. LOCK TABLE
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 6479dcb..daf3d81 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -23,11 +23,15 @@
 #include "utils/acl.h"
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
+#include "rewrite/rewriteHandler.h"
+#include "access/heapam.h"
+#include "nodes/nodeFuncs.h"
 
-static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait);
-static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode);
+static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid);
+static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode, Oid userid);
 static void RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid,
 			 Oid oldrelid, void *arg);
+static void LockViewRecurse(Oid reloid, Oid root_reloid, LOCKMODE lockmode, bool nowait);
 
 /*
  * LOCK TABLE
@@ -62,8 +66,10 @@ LockTableCommand(LockStmt *lockstmt)
 		  RangeVarCallbackForLockTable,
 		  (void *) >mode);
 
-		if (recurse)
-			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait);
+		if (get_rel_relkind(reloid) == RELKIND_VIEW)
+			LockViewRecurse(reloid, reloid, lockstmt->mode, lockstmt->nowait);
+		else if (recurse)
+			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId());
 	}
 }
 
@@ -86,15 +92,17 @@ Ran

Re: CURRENT OF causes an error when IndexOnlyScan is used

[Yugo Nagata]

On Mon, 12 Mar 2018 13:56:24 -0400
Tom Lane <t...@sss.pgh.pa.us> wrote:

> Anastasia Lubennikova <a.lubennik...@postgrespro.ru> writes:
> > [ return_heaptuple_in_btree_indexonlyscan_v2.patch ]
> 
> I took a quick look at this, but I'm concerned about a couple of points:
> 
> 1. What's the performance penalty of forming (and then deforming) the
> added heap tuple?  We'd be paying it for every index-only scan, whether
> or not any CURRENT OF fetch ever happened.
> 
> 2. The constructed tuple provides tableoid and ctid all right, but it'd
> still have garbage values for other system columns.  As the code stands,
> we will properly error out if some attempt is made to fetch any of those
> other columns, but with this change we'd just return the garbage.  This
> is a minor point, but it doesn't seem negligible to me; it might've been
> hard to identify the bug at hand if we'd not had the cross-check of not
> building a heap tuple.

In addition, this patch fixes only nbtree indexes, but the simillar problem
will occur also on GIST indexes which support index-only scan. If we resolve
this bug by fixing index access methods, I think we need to fix all existing
indexes that support index-only scan and also describe the specification in
the documents, comments, or README, etc. for future indexes.
 
> At this point Yugo-san's original patch is starting to look more
> attractive.  It's still ugly, but at least it's not imposing a performance
> cost on unrelated queries.

I would like to elaborate my patch if needed and possible. Any suggestion
would be appriciated.

Thanks,

> 
>   regards, tom lane



-- 
Yugo Nagata <nag...@sraoss.co.jp>

Re: [PATCH] GET DIAGNOSTICS FUNCTION_NAME

[Yugo Nagata]

On Thu, 1 Mar 2018 14:29:58 -0800
Andres Freund <and...@anarazel.de> wrote:

> Hi,
> 
> On 2018-01-11 11:03:26 +0900, Yugo Nagata wrote:
> > However, I don't inisist on this patch, so If anyone other don't need this
> > feature, I'll withdraw this.
> 
> Given this is where the discussion dried up more than a month ago I'm
> inclined to mark this as rejected unless somebody wants to argue
> otherwise?

I have no objection.

Thans,

> 
> Greetings,
> 
> Andres Freund


-- 
Yugo Nagata <nag...@sraoss.co.jp>

Re: CURRENT OF causes an error when IndexOnlyScan is used

[Yugo Nagata]

On Wed, 31 Jan 2018 21:12:51 -0500
Tom Lane <t...@sss.pgh.pa.us> wrote:

> Yugo Nagata <nag...@sraoss.co.jp> writes:
> > I'm sorry the patch attached in the previous mail is broken and
> > not raises a compile error. I attached the fixed patch.
> 
> This patch is almost certainly wrong: you can't assume that the scan-level
> state matches the tuple we are currently processing at top level.  Any
> sort of delaying action, for instance a sort or materialize node in
> between, would break it.

In execCurrentOf(), when FOR UPDATE is not used, search_plan_tree() searches
through the PlanState tree for a scan node and if a sort or materialize node
(for example) is found it fails with the following error.

 ERROR cursor xxx is not a simply updatable scan of table yyy

So, I think what you concern would not occur by the patch as well as the orginal
code. However, I may be missing something. Could you explain more about this if 
so?

> 
> We need to either fix this aspect:
> 
> >> IndexOnlyScan returns a virtual tuple that doesn't have system
> >> column, so we can not get ctid in the same way of other plans.
> 
> or else disallow using IndexOnlyScan when the ctid is needed.

CURRENT OF is used after the scan is executed and a tuple is fetched,
so we can't know whether the ctid is needed or not in advance in this
case. We can raise an error message when CURRENT OF is used
for IndexOnlyScan plan, though.

Regards,

> 
>   regards, tom lane


-- 
Yugo Nagata <nag...@sraoss.co.jp>

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

On Thu, 01 Feb 2018 09:48:49 +0900 (JST)
Tatsuo Ishii <is...@sraoss.co.jp> wrote:

> > On Tue, Jan 30, 2018 at 6:48 PM, Tatsuo Ishii <is...@sraoss.co.jp> wrote:
> >> Looks good to me. If there's no objection, especially from Thomas
> >> Munro, I will mark this as "ready for committer".
> > 
> > No objection from me.
> 
> I marked this as "Ready for Committer".

Thank you for reviewing the patch!

Regards,

> 
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp


-- 
Yugo Nagata <nag...@sraoss.co.jp>

Re: CURRENT OF causes an error when IndexOnlyScan is used

[Yugo Nagata]

On Thu, 1 Feb 2018 01:33:49 +0900
Yugo Nagata <nag...@sraoss.co.jp> wrote:

I'm sorry the patch attached in the previous mail is broken and
not raises a compile error. I attached the fixed patch.

Regards,

> Hi,
> 
> I found that updating a cursor by using CURRENT OF causes the
> following error when the query is executed by IndexOnlyScan. 
> 
>  ERROR:  cannot extract system attribute from virtual tuple
> 
> IndexOnlyScan returns a virtual tuple that doesn't have system
> column, so we can not get ctid in the same way of other plans.
> However, the error message is not convinient and users would
> not understand why the error occurs.
> 
> Attached is a patch to fix this. By this fix, execCurrentOf
> get ctid from IndexScanDesc->xs_ctup.t_self when the plan is
> IndexOnlyScan, and it works sucessfully without errors.
> 
> 
> Here is the example of the error:
> 
> ===
> postgres=# create table test (i int primary key);
> CREATE TABLE
> postgres=# insert into test values(1);
> INSERT 0 1
> postgres=# set enable_seqscan to off;
> SET
> 
> postgres=# explain select * from test where i = 1;
> QUERY PLAN 
> ---
>  Index Only Scan using test_pkey on test  (cost=0.15..8.17 rows=1 width=4)
>Index Cond: (i = 1)
> (2 rows)
> 
> postgres=# begin;
> BEGIN
> postgres=# declare c cursor for select * from test where i = 1;
> DECLARE CURSOR
> postgres=# fetch from c;
>  i 
> ---
>  1
> (1 row)
> 
> postgres=# update test set i=i+1 where current of c;
> ERROR:  cannot extract system attribute from virtual tuple
> ===
> 
> The patch fixes the error and allows this update successfully.
> 
> Regards,
> 
> -- 
> Yugo Nagata <nag...@sraoss.co.jp>


-- 
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/src/backend/executor/execCurrent.c b/src/backend/executor/execCurrent.c
index ce7d4ac..aa2da16 100644
--- a/src/backend/executor/execCurrent.c
+++ b/src/backend/executor/execCurrent.c
@@ -19,6 +19,7 @@
 #include "utils/lsyscache.h"
 #include "utils/portal.h"
 #include "utils/rel.h"
+#include "access/relscan.h"
 
 
 static char *fetch_cursor_param_value(ExprContext *econtext, int paramId);
@@ -183,21 +184,35 @@ execCurrentOf(CurrentOfExpr *cexpr,
 		if (TupIsNull(scanstate->ss_ScanTupleSlot))
 			return false;
 
-		/* Use slot_getattr to catch any possible mistakes */
-		tuple_tableoid =
-			DatumGetObjectId(slot_getattr(scanstate->ss_ScanTupleSlot,
-		  TableOidAttributeNumber,
-		  ));
-		Assert(!lisnull);
-		tuple_tid = (ItemPointer)
-			DatumGetPointer(slot_getattr(scanstate->ss_ScanTupleSlot,
-		 SelfItemPointerAttributeNumber,
-		 ));
-		Assert(!lisnull);
-
-		Assert(tuple_tableoid == table_oid);
-
-		*current_tid = *tuple_tid;
+		/* In IndexOnlyScan case, the tuple stored in ss_ScanTupleSlot is a
+		 * virtual tuple that does not have ctid column, so we have to get TID
+		 * from xs_ctup.t_self. */
+		if (IsA(scanstate, IndexOnlyScanState))
+		{
+			IndexScanDesc scan = ((IndexOnlyScanState *)scanstate)->ioss_ScanDesc;
+
+			Assert(RelationGetRelid(scan.heapRelation) == table_oid);
+
+			*current_tid = scan->xs_ctup.t_self;
+		}
+		else
+		{
+			/* Use slot_getattr to catch any possible mistakes */
+			tuple_tableoid =
+DatumGetObjectId(slot_getattr(scanstate->ss_ScanTupleSlot,
+			  TableOidAttributeNumber,
+			  ));
+			Assert(!lisnull);
+			tuple_tid = (ItemPointer)
+DatumGetPointer(slot_getattr(scanstate->ss_ScanTupleSlot,
+			 SelfItemPointerAttributeNumber,
+			 ));
+			Assert(!lisnull);
+
+			Assert(tuple_tableoid == table_oid);
+
+			*current_tid = *tuple_tid;
+		}
 
 		return true;
 	}

CURRENT OF causes an error when IndexOnlyScan is used

[Yugo Nagata]

Hi,

I found that updating a cursor by using CURRENT OF causes the
following error when the query is executed by IndexOnlyScan. 

 ERROR:  cannot extract system attribute from virtual tuple

IndexOnlyScan returns a virtual tuple that doesn't have system
column, so we can not get ctid in the same way of other plans.
However, the error message is not convinient and users would
not understand why the error occurs.

Attached is a patch to fix this. By this fix, execCurrentOf
get ctid from IndexScanDesc->xs_ctup.t_self when the plan is
IndexOnlyScan, and it works sucessfully without errors.


Here is the example of the error:

===
postgres=# create table test (i int primary key);
CREATE TABLE
postgres=# insert into test values(1);
INSERT 0 1
postgres=# set enable_seqscan to off;
SET

postgres=# explain select * from test where i = 1;
QUERY PLAN 
---
 Index Only Scan using test_pkey on test  (cost=0.15..8.17 rows=1 width=4)
   Index Cond: (i = 1)
(2 rows)

postgres=# begin;
BEGIN
postgres=# declare c cursor for select * from test where i = 1;
DECLARE CURSOR
postgres=# fetch from c;
 i 
---
 1
(1 row)

postgres=# update test set i=i+1 where current of c;
ERROR:  cannot extract system attribute from virtual tuple
===

The patch fixes the error and allows this update successfully.

Regards,

-- 
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/src/backend/executor/execCurrent.c b/src/backend/executor/execCurrent.c
index ce7d4ac..b37cf3d 100644
--- a/src/backend/executor/execCurrent.c
+++ b/src/backend/executor/execCurrent.c
@@ -19,6 +19,7 @@
 #include "utils/lsyscache.h"
 #include "utils/portal.h"
 #include "utils/rel.h"
+#include "access/relscan.h"
 
 
 static char *fetch_cursor_param_value(ExprContext *econtext, int paramId);
@@ -183,21 +184,35 @@ execCurrentOf(CurrentOfExpr *cexpr,
 		if (TupIsNull(scanstate->ss_ScanTupleSlot))
 			return false;
 
-		/* Use slot_getattr to catch any possible mistakes */
-		tuple_tableoid =
-			DatumGetObjectId(slot_getattr(scanstate->ss_ScanTupleSlot,
-		  TableOidAttributeNumber,
-		  ));
-		Assert(!lisnull);
-		tuple_tid = (ItemPointer)
-			DatumGetPointer(slot_getattr(scanstate->ss_ScanTupleSlot,
-		 SelfItemPointerAttributeNumber,
-		 ));
-		Assert(!lisnull);
-
-		Assert(tuple_tableoid == table_oid);
-
-		*current_tid = *tuple_tid;
+		/* In IndexOnlyScan case, the tuple stored in ss_ScanTupleSlot is a
+		 * virtual tuple that does not have ctid column, so we have to get TID
+		 * from xs_ctup.t_self. */
+		if (IsA(scanstate, IndexOnlyScanState))
+		{
+			IndexScanDesc scan = ((IndexOnlyScanState *)scanstate)->ScanDesc;
+
+			Assert(RelationGetRelid(scan.heapRelation) == table_oid);
+
+			*current_tid = scan->xs_ctup.t_self;
+		}
+		else
+		{
+			/* Use slot_getattr to catch any possible mistakes */
+			tuple_tableoid =
+DatumGetObjectId(slot_getattr(scanstate->ss_ScanTupleSlot,
+			  TableOidAttributeNumber,
+			  ));
+			Assert(!lisnull);
+			tuple_tid = (ItemPointer)
+DatumGetPointer(slot_getattr(scanstate->ss_ScanTupleSlot,
+			 SelfItemPointerAttributeNumber,
+			 ));
+			Assert(!lisnull);
+
+			Assert(tuple_tableoid == table_oid);
+
+			*current_tid = *tuple_tid;
+		}
 
 		return true;
 	}

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

On Tue, 30 Jan 2018 19:21:04 +1300
Thomas Munro <thomas.mu...@enterprisedb.com> wrote:

> On Tue, Jan 30, 2018 at 6:48 PM, Tatsuo Ishii <is...@sraoss.co.jp> wrote:
> >>> You need to DROP VIEW lock_view4 and lock_view5 in the regression
> >>> test as well.
> >>
> >> Thank you for reviewing the patch.
> >>
> >> I fixed this and attached a updated patch v6.
> >
> > Looks good to me. If there's no objection, especially from Thomas
> > Munro, I will mark this as "ready for committer".
> 
> About the idea:  it makes some kind of sense to me that we should lock
> the underlying table, in all the same cases that you could do DML on
> the view automatically.  I wonder if this is a problem for the
> soundness:  "Tables appearing in a subquery are ignored and not
> locked."  I can imagine using this for making backwards-compatible
> schema changes, in which case the LOCK-based transaction isolation
> techniques might benefit from this behaviour.  I'd be interested to
> hear about the ideal use case you have in mind.

I think the use case is almost similar to that of auto-updatable views.
There are some purpose to use views, for example 1) preventing from
modifying application due to schema changes, 2) protecting the underlying
table from users without proper privileges, 3) making a shorthand of a
query with complex WHERE condition. When these are updatable views and
users need transaction isolation during updating them, I think the lockable
views feature is benefitical because users don't need to refer to the
underlying table. Users might don't know the underlying table, or even
might not have the privilege to lock this.

> About the patch:  I didn't study it in detail.  It builds, has
> documentation and passes all tests.  Would it be a good idea to add an
> isolation test to show that the underlying relation is actually
> locked?

Whether the underlying relation is actually locked or not is confirmed
in the regression test using pg_locks, so I don't believe that we need
to add an isolation test.
 
> Typo:
> 
> + /* Check permissions with the view owner's priviledge. */
> 
> s/priviledge/privilege/
> 
> Grammar:
> 
> +/*
> + * Check whether the view is lockable.
> + *
> + * Currently, only auto-updatable views can be locked, that is,
> + * views whose definition are simple and that doesn't have
> + * instead of rules or triggers are lockable.
> 
> s/definition are simple and that doesn't/definition is simple and that don't/
> s/instead of/INSTEAD OF/ ?

Thank you for pointing out these. I attached the fixed patch.

Regards,
 
> -- 
> Thomas Munro
> http://www.enterprisedb.com


-- 
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
index b2c7203..6ddd128 100644
--- a/doc/src/sgml/ref/lock.sgml
+++ b/doc/src/sgml/ref/lock.sgml
@@ -46,6 +46,14 @@ LOCK [ TABLE ] [ ONLY ] name [ * ]
   
 
   
+   Automatically updatable views (see )
+   that do not have INSTEAD OF triggers or
+   INSTEAD rules are also lockable. When a view is
+   locked, its base relations are also locked recursively with the same
+   lock mode. Tables appearing in a subquery are ignored and not locked.
+  
+
+  
When acquiring locks automatically for commands that reference
tables, PostgreSQL always uses the least
restrictive lock mode possible. LOCK TABLE
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 6479dcb..f6b5962 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -19,15 +19,20 @@
 #include "commands/lockcmds.h"
 #include "miscadmin.h"
 #include "parser/parse_clause.h"
+#include "parser/parsetree.h"
 #include "storage/lmgr.h"
 #include "utils/acl.h"
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
+#include "rewrite/rewriteHandler.h"
+#include "access/heapam.h"
 
-static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait);
-static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode);
+static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid);
+static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode, Oid userid);
 static void RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid,
 			 Oid oldrelid, void *arg);
+static void LockViewRecurse(Oid reloid, Oid root_reloid, LOCKMODE lockmode, bool nowait);
+static void LockViewCheck(Relation view, Query *viewquery);
 
 /*
  * LOCK TABLE
@@ -62,8 +67,10 @@ LockTableCommand(LockStmt *lockstmt)
 		  RangeVarCallbackForLockTable,
 		  (void *) >mode);
 
-		if (recurse)
-			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait);
+		if (get_rel_relkind(reloid)

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

On Tue, 30 Jan 2018 13:58:30 +0900 (JST)
Tatsuo Ishii <is...@sraoss.co.jp> wrote:

> > Attached is the updated patch v5 including fixing SGML and rebase to HEAD.
> 
> You need to DROP VIEW lock_view4 and lock_view5 in the regression
> test as well.

Thank you for reviewing the patch.

I fixed this and attached a updated patch v6.

Regards,

> 
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp


-- 
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
index b2c7203..6ddd128 100644
--- a/doc/src/sgml/ref/lock.sgml
+++ b/doc/src/sgml/ref/lock.sgml
@@ -46,6 +46,14 @@ LOCK [ TABLE ] [ ONLY ] name [ * ]
   
 
   
+   Automatically updatable views (see )
+   that do not have INSTEAD OF triggers or
+   INSTEAD rules are also lockable. When a view is
+   locked, its base relations are also locked recursively with the same
+   lock mode. Tables appearing in a subquery are ignored and not locked.
+  
+
+  
When acquiring locks automatically for commands that reference
tables, PostgreSQL always uses the least
restrictive lock mode possible. LOCK TABLE
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 6479dcb..7ae3a84 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -19,15 +19,20 @@
 #include "commands/lockcmds.h"
 #include "miscadmin.h"
 #include "parser/parse_clause.h"
+#include "parser/parsetree.h"
 #include "storage/lmgr.h"
 #include "utils/acl.h"
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
+#include "rewrite/rewriteHandler.h"
+#include "access/heapam.h"
 
-static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait);
-static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode);
+static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid);
+static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode, Oid userid);
 static void RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid,
 			 Oid oldrelid, void *arg);
+static void LockViewRecurse(Oid reloid, Oid root_reloid, LOCKMODE lockmode, bool nowait);
+static void LockViewCheck(Relation view, Query *viewquery);
 
 /*
  * LOCK TABLE
@@ -62,8 +67,10 @@ LockTableCommand(LockStmt *lockstmt)
 		  RangeVarCallbackForLockTable,
 		  (void *) >mode);
 
-		if (recurse)
-			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait);
+		if (get_rel_relkind(reloid) == RELKIND_VIEW)
+			LockViewRecurse(reloid, reloid, lockstmt->mode, lockstmt->nowait);
+		else if (recurse)
+			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId());
 	}
 }
 
@@ -86,15 +93,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
 		return;	/* woops, concurrently dropped; no permissions
  * check */
 
-	/* Currently, we only allow plain tables to be locked */
-	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
+
+	/* Currently, we only allow plain tables or auto-updatable views to be locked */
+	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE &&
+		relkind != RELKIND_VIEW)
 		ereport(ERROR,
 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table",
+ errmsg("\"%s\" is not a table or view",
 		rv->relname)));
 
 	/* Check permissions. */
-	aclresult = LockTableAclCheck(relid, lockmode);
+	aclresult = LockTableAclCheck(relid, lockmode, GetUserId());
 	if (aclresult != ACLCHECK_OK)
 		aclcheck_error(aclresult, get_relkind_objtype(get_rel_relkind(relid)), rv->relname);
 }
@@ -107,7 +116,7 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
  * multiply-inheriting children more than once, but that's no problem.
  */
 static void
-LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait)
+LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid)
 {
 	List	   *children;
 	ListCell   *lc;
@@ -120,7 +129,7 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait)
 		AclResult	aclresult;
 
 		/* Check permissions before acquiring the lock. */
-		aclresult = LockTableAclCheck(childreloid, lockmode);
+		aclresult = LockTableAclCheck(childreloid, lockmode, userid);
 		if (aclresult != ACLCHECK_OK)
 		{
 			char	   *relname = get_rel_name(childreloid);
@@ -157,15 +166,82 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait)
 			continue;
 		}
 
-		LockTableRecurse(childreloid, lockmode, nowait);
+		LockTableRecurse(childreloid, lockmode, nowait, userid);
 	}
 }
 
 /*
+ * Apply LOCK TABLE recursively over a view
+ */
+static void
+LockViewRecurse(Oid reloid, Oid root_reloid, LO

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

On Fri, 26 Jan 2018 21:30:49 +0900
Yugo Nagata <nag...@sraoss.co.jp> wrote:

> On Thu, 25 Jan 2018 20:51:41 +1300
> Thomas Munro <thomas.mu...@enterprisedb.com> wrote:
> 
> > On Sun, Dec 31, 2017 at 11:57 PM, Yugo Nagata <nag...@sraoss.co.jp> wrote:
> > > On Fri, 29 Dec 2017 23:39:39 +0900 (JST)
> > > Tatsuo Ishii <is...@sraoss.co.jp> wrote:
> > >> Your addition to the doc:
> > >> +   Automatically updatable views (see )
> > >> +   that do not have INSTEAD OF triggers or 
> > >> INSTEAD
> > >> +   rules are also lockable. When a view is locked, its base relations 
> > >> are
> > >> +   also locked recursively with the same lock mode.
> > >
> > > I added this point to the documentation.
> > 
> > +   Automatically updatable views (see )
> > +   that do not have INSTEAD OF triggers or INSTEAD
> 
> Thank you for pointing out this.
> 
> Also, I've noticed this patch cannot be applied to the HEAD, so I'll fix
> them together and update the patch.

Attached is the updated patch v5 including fixing SGML and rebase to HEAD.

Regards,

> 
> > 
> > Tthe documentation doesn't build: you now need to say 
> > instead of , and you need to say .
> > 
> > -- 
> > Thomas Munro
> > http://www.enterprisedb.com
> > 
> 
> 
> -- 
> Yugo Nagata <nag...@sraoss.co.jp>
> 


-- 
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
index b2c7203..6ddd128 100644
--- a/doc/src/sgml/ref/lock.sgml
+++ b/doc/src/sgml/ref/lock.sgml
@@ -46,6 +46,14 @@ LOCK [ TABLE ] [ ONLY ] name [ * ]
   
 
   
+   Automatically updatable views (see )
+   that do not have INSTEAD OF triggers or
+   INSTEAD rules are also lockable. When a view is
+   locked, its base relations are also locked recursively with the same
+   lock mode. Tables appearing in a subquery are ignored and not locked.
+  
+
+  
When acquiring locks automatically for commands that reference
tables, PostgreSQL always uses the least
restrictive lock mode possible. LOCK TABLE
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 6479dcb..7ae3a84 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -19,15 +19,20 @@
 #include "commands/lockcmds.h"
 #include "miscadmin.h"
 #include "parser/parse_clause.h"
+#include "parser/parsetree.h"
 #include "storage/lmgr.h"
 #include "utils/acl.h"
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
+#include "rewrite/rewriteHandler.h"
+#include "access/heapam.h"
 
-static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait);
-static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode);
+static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid);
+static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode, Oid userid);
 static void RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid,
 			 Oid oldrelid, void *arg);
+static void LockViewRecurse(Oid reloid, Oid root_reloid, LOCKMODE lockmode, bool nowait);
+static void LockViewCheck(Relation view, Query *viewquery);
 
 /*
  * LOCK TABLE
@@ -62,8 +67,10 @@ LockTableCommand(LockStmt *lockstmt)
 		  RangeVarCallbackForLockTable,
 		  (void *) >mode);
 
-		if (recurse)
-			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait);
+		if (get_rel_relkind(reloid) == RELKIND_VIEW)
+			LockViewRecurse(reloid, reloid, lockstmt->mode, lockstmt->nowait);
+		else if (recurse)
+			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId());
 	}
 }
 
@@ -86,15 +93,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
 		return;	/* woops, concurrently dropped; no permissions
  * check */
 
-	/* Currently, we only allow plain tables to be locked */
-	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
+
+	/* Currently, we only allow plain tables or auto-updatable views to be locked */
+	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE &&
+		relkind != RELKIND_VIEW)
 		ereport(ERROR,
 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table",
+ errmsg("\"%s\" is not a table or view",
 		rv->relname)));
 
 	/* Check permissions. */
-	aclresult = LockTableAclCheck(relid, lockmode);
+	aclresult = LockTableAclCheck(relid, lockmode, GetUserId());
 	if (aclresult != ACLCHECK_OK)
 		aclcheck_error(aclresult, get_relkind_objtype(get_rel_relkind(relid)), rv->relname);
 }
@@ -107,7 +116,7 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid r

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

On Thu, 25 Jan 2018 20:51:41 +1300
Thomas Munro <thomas.mu...@enterprisedb.com> wrote:

> On Sun, Dec 31, 2017 at 11:57 PM, Yugo Nagata <nag...@sraoss.co.jp> wrote:
> > On Fri, 29 Dec 2017 23:39:39 +0900 (JST)
> > Tatsuo Ishii <is...@sraoss.co.jp> wrote:
> >> Your addition to the doc:
> >> +   Automatically updatable views (see )
> >> +   that do not have INSTEAD OF triggers or INSTEAD
> >> +   rules are also lockable. When a view is locked, its base relations are
> >> +   also locked recursively with the same lock mode.
> >
> > I added this point to the documentation.
> 
> +   Automatically updatable views (see )
> +   that do not have INSTEAD OF triggers or INSTEAD

Thank you for pointing out this.

Also, I've noticed this patch cannot be applied to the HEAD, so I'll fix
them together and update the patch.

> 
> Tthe documentation doesn't build: you now need to say 
> instead of , and you need to say .
> 
> -- 
> Thomas Munro
> http://www.enterprisedb.com
> 


-- 
Yugo Nagata <nag...@sraoss.co.jp>

Re: [PATCH] GET DIAGNOSTICS FUNCTION_NAME

[Yugo Nagata]

On Sun, 31 Dec 2017 11:57:02 -0500
Tom Lane <t...@sss.pgh.pa.us> wrote:

> Yugo Nagata <nag...@sraoss.co.jp> writes:
> > Attached is a patch to implement a feature to get the current function
> > name by GET DIAGNOSTICS in PL/pgSQL function.
> 
> While this is certainly not a very large patch, it's still code that
> we'd have to maintain forever, so I think it's appropriate to ask
> some harder questions before accepting it.
> 
> 1. I'm having a hard time visualizing an actual concrete use case for
> this --- exactly when would a function not know its own name?  Neither
> your "our client wanted it" justification nor the cited stackoverflow
> question provide anything close to an adequate rationale.  I can think of
> concrete uses for an operation like "give me the name of my immediate
> caller", but that's not what this is.

Our client's use case was mainly to output debug messages at begining and
end of functions by using the same code. In addition, names of cursors
declared in the function were based on the function name, and they wanted
to get the function name to handle cursors.

However, I don't inisist on this patch, so If anyone other don't need this
feature, I'll withdraw this.

Regards,

> 
> 2. The specific semantics you've chosen --- in effect, regprocedureout
> results --- seem to be more because that was already available than
> anything else.  I can imagine wanting just the bare name, or the
> schema-qualified name, or even the numeric OID (if we're in the
> business of introspection, being able to look up the function's own
> pg_proc entry might be useful).  I'm not proposing that we offer
> all those variants, certainly, but without concrete use cases it's
> pretty hard to be sure we picked the most useful behavior.
> 
> 3. In connection with #2, I'm dubious that FUNCTION_NAME is le mot
> juste, because that would seem to imply that it is just the name,
> which it isn't.  If we stick with the regprocedureout semantics
> I'd be inclined to propose FUNCTION_SIGNATURE.
> 
>   regards, tom lane
> 


-- 
Yugo Nagata <nag...@sraoss.co.jp>

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

Hi,

The updated patch is attached.

On Fri, 29 Dec 2017 23:39:39 +0900 (JST)
Tatsuo Ishii <is...@sraoss.co.jp> wrote:

 
> The patch produces a warning.
> 
> /home/t-ishii/lock_view-v3.patch:542: trailing whitespace.
> -- Verify that we  can lock a auto-updatable views 
> warning: 1 line adds whitespace errors.

Fixed.

> 
> Your addition to the doc:
> +   Automatically updatable views (see )
> +   that do not have INSTEAD OF triggers or INSTEAD
> +   rules are also lockable. When a view is locked, its base relations are
> +   also locked recursively with the same lock mode.
> 
> does not mention about the point:
> 
> >> >> > 1) Leave as it is (ignore tables appearing in a subquery)

I added this point to the documentation.


Regards,

> 
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp


-- 
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
index b2c7203..c2ed481 100644
--- a/doc/src/sgml/ref/lock.sgml
+++ b/doc/src/sgml/ref/lock.sgml
@@ -46,6 +46,14 @@ LOCK [ TABLE ] [ ONLY ] name [ * ]
   
 
   
+   Automatically updatable views (see )
+   that do not have INSTEAD OF triggers or INSTEAD
+   rules are also lockable. When a view is locked, its base relations are
+   also locked recursively with the same lock mode. Tables appearing in a
+   subquery are ignored and not locked.
+  
+
+  
When acquiring locks automatically for commands that reference
tables, PostgreSQL always uses the least
restrictive lock mode possible. LOCK TABLE
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 9fe9e02..80e00f7 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -19,15 +19,20 @@
 #include "commands/lockcmds.h"
 #include "miscadmin.h"
 #include "parser/parse_clause.h"
+#include "parser/parsetree.h"
 #include "storage/lmgr.h"
 #include "utils/acl.h"
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
+#include "rewrite/rewriteHandler.h"
+#include "access/heapam.h"
 
-static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait);
-static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode);
+static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid);
+static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode, Oid userid);
 static void RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid,
 			 Oid oldrelid, void *arg);
+static void LockViewRecurse(Oid reloid, Oid root_reloid, LOCKMODE lockmode, bool nowait);
+static void LockViewCheck(Relation view, Query *viewquery);
 
 /*
  * LOCK TABLE
@@ -62,8 +67,10 @@ LockTableCommand(LockStmt *lockstmt)
 		  RangeVarCallbackForLockTable,
 		  (void *) >mode);
 
-		if (recurse)
-			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait);
+		if (get_rel_relkind(reloid) == RELKIND_VIEW)
+			LockViewRecurse(reloid, reloid, lockstmt->mode, lockstmt->nowait);
+		else if (recurse)
+			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId());
 	}
 }
 
@@ -86,15 +93,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
 		return;	/* woops, concurrently dropped; no permissions
  * check */
 
-	/* Currently, we only allow plain tables to be locked */
-	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
+
+	/* Currently, we only allow plain tables or auto-updatable views to be locked */
+	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE &&
+		relkind != RELKIND_VIEW)
 		ereport(ERROR,
 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table",
+ errmsg("\"%s\" is not a table or view",
 		rv->relname)));
 
 	/* Check permissions. */
-	aclresult = LockTableAclCheck(relid, lockmode);
+	aclresult = LockTableAclCheck(relid, lockmode, GetUserId());
 	if (aclresult != ACLCHECK_OK)
 		aclcheck_error(aclresult, ACL_KIND_CLASS, rv->relname);
 }
@@ -107,7 +116,7 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
  * multiply-inheriting children more than once, but that's no problem.
  */
 static void
-LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait)
+LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid)
 {
 	List	   *children;
 	ListCell   *lc;
@@ -120,7 +129,7 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait)
 		AclResult	aclresult;
 
 		/* Check permissions before acquiring the lock. */
-		aclresult = LockTableAclCheck(childreloid, lockmode);
+		aclresult = LockTableAclCheck(childreloid, lockmode, userid);
 		if (aclresult 

Re: Fix a Oracle-compatible instr function in the documentation

[Yugo Nagata]

On Sun, 31 Dec 2017 17:52:49 +0900 (JST)
Tatsuo Ishii <is...@sraoss.co.jp> wrote:

> >> Hi,
> >> 
> >> Attached is a patch to fix a very trivial issue of the documentation.
> >> 
> >> The documentation of PL/pgSQL provides sample codes of Oracle-compatible
> >> instr functions. However, the behaviour is a little differet.
> >> Oracle's instr raises an error when the forth argument value is less than
> >> zero, but the sample code returns zero. This patch fixes this.
> > 
> > Do we need treat this as a bug fix? If so, do we need to back patch as
> > well?

This is a little improvement of the documentation to reduce confusion of users
who work on migration from Oracle. I don't know whether this need tobe back
patched, so I'll leave a decision up to commiters.

> 
> I have added this to CF 2018-01.

Thank you.

> 
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp


-- 
Yugo Nagata <nag...@sraoss.co.jp>

Re: [PATCH] GET DIAGNOSTICS FUNCTION_NAME

[Yugo Nagata]

On Sun, 31 Dec 2017 17:54:06 +0900 (JST)
Tatsuo Ishii <is...@sraoss.co.jp> wrote:

> >> Attached is a patch to implement a feature to get the current function
> >> name by GET DIAGNOSTICS in PL/pgSQL function.
> > 
> > Could you add it to the nexf CF, I have not seen it there? Maybe the
> > deadline is tonight...
> 
> I have added this to the next CF.

Thank you.

> 
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp


-- 
Yugo Nagata <nag...@sraoss.co.jp>

Fix a Oracle-compatible instr function in the documentation

[Yugo Nagata]

Hi,

Attached is a patch to fix a very trivial issue of the documentation.

The documentation of PL/pgSQL provides sample codes of Oracle-compatible
instr functions. However, the behaviour is a little differet.
Oracle's instr raises an error when the forth argument value is less than
zero, but the sample code returns zero. This patch fixes this.

Regards,

-- 
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 7d23ed4..5a67c38 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -5676,6 +5676,10 @@ DECLARE
 length integer;
 ss_length integer;
 BEGIN
+IF occur_index = 0 THEN
+RAISE 'argument ''%'' is out of range', occur_index USING ERRCODE = '22003';
+END IF;
+
 IF beg_index  0 THEN
 temp_str := substring(string FROM beg_index);
 pos := position(string_to_search IN temp_str);

[PATCH] GET DIAGNOSTICS FUNCTION_NAME

[Yugo Nagata]

Hi,

Attached is a patch to implement a feature to get the current function
name by GET DIAGNOSTICS in PL/pgSQL function.

Currentyly, we can get call stack by GET DIAGNOSTICS PG_CONTEXT, but
we cannot get the function name directly. One of our clients wanted
this feature for debugging, and this was realized by creating a 
function that extracts the function name string from call stack. 
However, the overhead of function calls was not small, and it
caused performance regression. 

I found that there are other needs for this feature[1], so I have
implemented this.

[1] 
https://stackoverflow.com/questions/12611596/getting-name-of-the-current-function-inside-of-the-function-with-plpgsql

Example:

postgres=# CREATE FUNCTION test() RETURNS void 
LANGUAGE plpgsql AS $$
DECLARE t text;
BEGIN
GET DIAGNOSTICS t = FUNCTION_NAME;
RAISE INFO 'function name: %', t;
END;
$$;
CREATE FUNCTION

postgres=# select test();
INFO:  function name: test()
 test 
--
 
(1 row)

Regards,

-- 
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 7d23ed4..c0daa14 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -1506,6 +1506,12 @@ GET DIAGNOSTICS integer_var = ROW_COUNT;
  line(s) of text describing the current call stack
   (see )
 
+
+ FUNCTION_NAME
+ text
+ text describing the current function signature 
+  (see )
+

   
  
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index dd575e7..4404aac 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -1812,6 +1812,11 @@ exec_stmt_getdiag(PLpgSQL_execstate *estate, PLpgSQL_stmt_getdiag *stmt)
 }
 break;
 
+			case PLPGSQL_GETDIAG_FUNCTION_NAME:
+exec_assign_c_string(estate, var,
+	 estate->func->fn_signature);
+break;
+
 			default:
 elog(ERROR, "unrecognized diagnostic item kind: %d",
 	 diag_item->kind);
diff --git a/src/pl/plpgsql/src/pl_funcs.c b/src/pl/plpgsql/src/pl_funcs.c
index be779b6..9d9a493 100644
--- a/src/pl/plpgsql/src/pl_funcs.c
+++ b/src/pl/plpgsql/src/pl_funcs.c
@@ -323,6 +323,8 @@ plpgsql_getdiag_kindname(PLpgSQL_getdiag_kind kind)
 			return "TABLE_NAME";
 		case PLPGSQL_GETDIAG_SCHEMA_NAME:
 			return "SCHEMA_NAME";
+		case PLPGSQL_GETDIAG_FUNCTION_NAME:
+			return "FUNCTION_NAME";
 	}
 
 	return "unknown";
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index e802440..adf20e7 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -287,6 +287,7 @@ static	void			check_raise_parameters(PLpgSQL_stmt_raise *stmt);
 %token 	K_FOREACH
 %token 	K_FORWARD
 %token 	K_FROM
+%token 	K_FUNCTION_NAME
 %token 	K_GET
 %token 	K_HINT
 %token 	K_IF
@@ -949,6 +950,7 @@ stmt_getdiag	: K_GET getdiag_area_opt K_DIAGNOSTICS getdiag_list ';'
 /* these fields are disallowed in stacked case */
 case PLPGSQL_GETDIAG_ROW_COUNT:
 case PLPGSQL_GETDIAG_RESULT_OID:
+case PLPGSQL_GETDIAG_FUNCTION_NAME:
 	if (new->is_stacked)
 		ereport(ERROR,
 (errcode(ERRCODE_SYNTAX_ERROR),
@@ -1065,6 +1067,9 @@ getdiag_item :
 K_SCHEMA_NAME, "schema_name"))
 			$$ = PLPGSQL_GETDIAG_SCHEMA_NAME;
 		else if (tok_is_keyword(tok, ,
+K_FUNCTION_NAME, "function_name"))
+			$$ = PLPGSQL_GETDIAG_FUNCTION_NAME;
+		else if (tok_is_keyword(tok, ,
 K_RETURNED_SQLSTATE, "returned_sqlstate"))
 			$$ = PLPGSQL_GETDIAG_RETURNED_SQLSTATE;
 		else
@@ -2407,6 +2412,7 @@ unreserved_keyword	:
 | K_FETCH
 | K_FIRST
 | K_FORWARD
+| K_FUNCTION_NAME
 | K_GET
 | K_HINT
 | K_IMPORT
diff --git a/src/pl/plpgsql/src/pl_scanner.c b/src/pl/plpgsql/src/pl_scanner.c
index 553be8c..2b624f6 100644
--- a/src/pl/plpgsql/src/pl_scanner.c
+++ b/src/pl/plpgsql/src/pl_scanner.c
@@ -127,6 +127,7 @@ static const ScanKeyword unreserved_keywords[] = {
 	PG_KEYWORD("fetch", K_FETCH, UNRESERVED_KEYWORD)
 	PG_KEYWORD("first", K_FIRST, UNRESERVED_KEYWORD)
 	PG_KEYWORD("forward", K_FORWARD, UNRESERVED_KEYWORD)
+	PG_KEYWORD("function_name", K_FUNCTION_NAME, UNRESERVED_KEYWORD)
 	PG_KEYWORD("get", K_GET, UNRESERVED_KEYWORD)
 	PG_KEYWORD("hint", K_HINT, UNRESERVED_KEYWORD)
 	PG_KEYWORD("import", K_IMPORT, UNRESERVED_KEYWORD)
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index 43d7d7d..f515f50 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -136,7 +136,8 @@ typedef enum PLpgSQL_getdiag_kind
 	PLPGSQL_GETDIAG_DATATYPE_NAME,
 	PLPGSQL_GETDIAG_MESSAGE_TEXT,
 	PLPGSQL_GETDIAG_TABLE_NAME,
-	PLPGSQL_GETDIAG_SCHEMA_NAME
+	PLPGSQL_GETDIAG_SCHEMA_NAME,
+	PLPGSQL_GETDIAG_FUNCTION_NAME
 } PLpgSQL_getdiag_kind;
 
 /*

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

On Thu, 28 Dec 2017 09:29:11 +0900 (JST)
Tatsuo Ishii <is...@sraoss.co.jp> wrote:

> > I didn't want to change the interface of view_query_is_auto_updatable()
> > because this might be called from other third-patry software, so I renamed
> > this function to view_query_is_auto_updatable_or_lockable() and added the 
> > flag
> > to this. I created view_query_is_auto_updatable() as a wrapper of this 
> > function.
> > I also made view_query_is_lockable() that returns a other message than 
> > view_query_is_auto_updatable().
> > 
> >> On Tue, 17 Oct 2017 11:59:05 +0900 (JST)
> >> Tatsuo Ishii <is...@sraoss.co.jp> wrote:
> >> > 1) Leave as it is (ignore tables appearing in a subquery)
> >> > 
> >> > 2) Lock all tables including in a subquery
> >> > 
> >> > 3) Check subquery in the view 
> > 
> >> > So it seem #1 is the most reasonable way to deal with the problem
> >> > assuming that it's user's responsibility to take appropriate locks on
> >> > the tables in the subquery.
> > 
> > I adopted #1 and I didn't change anything about this.
> 
> Looks good to me except that the patch lacks documents and the
> regression test needs more cases. For example, it needs a test for the
> case #1 above: probably using pg_locks to make sure that the tables
> appearing in the subquery do not hold locks.

Attached is the update patch, v3. I add some regression test and
the documentation.

> 
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp


-- 
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
index b2c7203..986ae71 100644
--- a/doc/src/sgml/ref/lock.sgml
+++ b/doc/src/sgml/ref/lock.sgml
@@ -46,6 +46,13 @@ LOCK [ TABLE ] [ ONLY ] name [ * ]
   
 
   
+   Automatically updatable views (see )
+   that do not have INSTEAD OF triggers or INSTEAD
+   rules are also lockable. When a view is locked, its base relations are
+   also locked recursively with the same lock mode.
+  
+
+  
When acquiring locks automatically for commands that reference
tables, PostgreSQL always uses the least
restrictive lock mode possible. LOCK TABLE
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 9fe9e02..80e00f7 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -19,15 +19,20 @@
 #include "commands/lockcmds.h"
 #include "miscadmin.h"
 #include "parser/parse_clause.h"
+#include "parser/parsetree.h"
 #include "storage/lmgr.h"
 #include "utils/acl.h"
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
+#include "rewrite/rewriteHandler.h"
+#include "access/heapam.h"
 
-static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait);
-static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode);
+static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid);
+static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode, Oid userid);
 static void RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid,
 			 Oid oldrelid, void *arg);
+static void LockViewRecurse(Oid reloid, Oid root_reloid, LOCKMODE lockmode, bool nowait);
+static void LockViewCheck(Relation view, Query *viewquery);
 
 /*
  * LOCK TABLE
@@ -62,8 +67,10 @@ LockTableCommand(LockStmt *lockstmt)
 		  RangeVarCallbackForLockTable,
 		  (void *) >mode);
 
-		if (recurse)
-			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait);
+		if (get_rel_relkind(reloid) == RELKIND_VIEW)
+			LockViewRecurse(reloid, reloid, lockstmt->mode, lockstmt->nowait);
+		else if (recurse)
+			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId());
 	}
 }
 
@@ -86,15 +93,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
 		return;	/* woops, concurrently dropped; no permissions
  * check */
 
-	/* Currently, we only allow plain tables to be locked */
-	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
+
+	/* Currently, we only allow plain tables or auto-updatable views to be locked */
+	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE &&
+		relkind != RELKIND_VIEW)
 		ereport(ERROR,
 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table",
+ errmsg("\"%s\" is not a table or view",
 		rv->relname)));
 
 	/* Check permissions. */
-	aclresult = LockTableAclCheck(relid, lockmode);
+	aclresult = LockTableAclCheck(relid, lockmode, GetUserId());
 	if (aclresult != ACLCHEC

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

Hi,

Attached is the updated patch.

On Mon, 16 Oct 2017 10:07:48 +0900 (JST)
Tatsuo Ishii <is...@sraoss.co.jp> wrote:
> >> > It would be nice if the message would be something like:
> >> > 
> >> > DETAIL:  Views that return aggregate functions are not lockable

> You could add a flag to view_query_is_auto_updatable() to switch the
> message between
> 
>  DETAIL:  Views that return aggregate functions are not automatically 
> updatable.
> 
> and
> 
>  DETAIL:  Views that return aggregate functions are not lockable

I didn't want to change the interface of view_query_is_auto_updatable()
because this might be called from other third-patry software, so I renamed
this function to view_query_is_auto_updatable_or_lockable() and added the flag
to this. I created view_query_is_auto_updatable() as a wrapper of this function.
I also made view_query_is_lockable() that returns a other message than 
view_query_is_auto_updatable().

> On Tue, 17 Oct 2017 11:59:05 +0900 (JST)
> Tatsuo Ishii <is...@sraoss.co.jp> wrote:
> > 1) Leave as it is (ignore tables appearing in a subquery)
> > 
> > 2) Lock all tables including in a subquery
> > 
> > 3) Check subquery in the view 

> > So it seem #1 is the most reasonable way to deal with the problem
> > assuming that it's user's responsibility to take appropriate locks on
> > the tables in the subquery.

I adopted #1 and I didn't change anything about this.

Regards,


-- 
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 9fe9e02..80e00f7 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -19,15 +19,20 @@
 #include "commands/lockcmds.h"
 #include "miscadmin.h"
 #include "parser/parse_clause.h"
+#include "parser/parsetree.h"
 #include "storage/lmgr.h"
 #include "utils/acl.h"
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
+#include "rewrite/rewriteHandler.h"
+#include "access/heapam.h"
 
-static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait);
-static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode);
+static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid);
+static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode, Oid userid);
 static void RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid,
 			 Oid oldrelid, void *arg);
+static void LockViewRecurse(Oid reloid, Oid root_reloid, LOCKMODE lockmode, bool nowait);
+static void LockViewCheck(Relation view, Query *viewquery);
 
 /*
  * LOCK TABLE
@@ -62,8 +67,10 @@ LockTableCommand(LockStmt *lockstmt)
 		  RangeVarCallbackForLockTable,
 		  (void *) >mode);
 
-		if (recurse)
-			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait);
+		if (get_rel_relkind(reloid) == RELKIND_VIEW)
+			LockViewRecurse(reloid, reloid, lockstmt->mode, lockstmt->nowait);
+		else if (recurse)
+			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId());
 	}
 }
 
@@ -86,15 +93,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
 		return;	/* woops, concurrently dropped; no permissions
  * check */
 
-	/* Currently, we only allow plain tables to be locked */
-	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
+
+	/* Currently, we only allow plain tables or auto-updatable views to be locked */
+	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE &&
+		relkind != RELKIND_VIEW)
 		ereport(ERROR,
 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table",
+ errmsg("\"%s\" is not a table or view",
 		rv->relname)));
 
 	/* Check permissions. */
-	aclresult = LockTableAclCheck(relid, lockmode);
+	aclresult = LockTableAclCheck(relid, lockmode, GetUserId());
 	if (aclresult != ACLCHECK_OK)
 		aclcheck_error(aclresult, ACL_KIND_CLASS, rv->relname);
 }
@@ -107,7 +116,7 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
  * multiply-inheriting children more than once, but that's no problem.
  */
 static void
-LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait)
+LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid)
 {
 	List	   *children;
 	ListCell   *lc;
@@ -120,7 +129,7 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait)
 		AclResult	aclresult;
 
 		/* Check permissions before acquiring the lock. */
-		aclresult = LockTableAclCheck(childreloid, lockmode);
+		aclresult = LockTableAclCheck(childreloid, lockmode, userid);
 		if (aclresult != ACLCHECK_OK)
 		{
 			char	   *relname = get_rel_name(childreloid);
@@ -157,15 +166,82 @@ LockTableRecurse(Oid

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

On Tue, 26 Dec 2017 22:22:33 +0900
Michael Paquier <michael.paqu...@gmail.com> wrote:

> On Tue, Dec 26, 2017 at 06:37:06PM +0900, Yugo Nagata wrote:
> > I have created a new entry in CF-2017-1 and registered this thread again.
> 
> Fine for me. Thanks for the update. And I guess that you are planning to
> send a new version before the beginning of the next commit fest using
> the feedback provided?

Yes. I'll update the patch according to Ishii-san's comments.

> --
> Michael


-- 
Yugo Nagata <nag...@sraoss.co.jp>

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

On Sat, 23 Dec 2017 09:44:30 +0900
Michael Paquier <michael.paqu...@gmail.com> wrote:

> On Fri, Dec 22, 2017 at 04:19:46PM +0900, Yugo Nagata wrote:
> > I was busy for and I could not work on this patch. After reading the
> > previous discussion, I still think the behavior of this patch would
> > be right. So, I would like to reregister to CF 2018-1. Do I need to
> > create a new entry on CF? or should I change the status to
> > "Moved to next CF"?
> 
> This is entirely up to you. It may make sense as well to spawn a new thread
> and mark the new patch set as v2, based on the feedback received for v1, as
> well as it could make sense to continue with this thread. If the move involves
> a complete patch rewrite with a rather different concept, a new thread is more
> adapted in my opinion.

Thank you for your comment.

I have created a new entry in CF-2017-1 and registered this thread again.

> -- 
> Michael


-- 
Yugo Nagata <nag...@sraoss.co.jp>

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

On Wed, 29 Nov 2017 11:29:36 +0900
Michael Paquier <michael.paqu...@gmail.com> wrote:

> On Fri, Oct 27, 2017 at 2:11 PM, Robert Haas <robertmh...@gmail.com> wrote:
> > On Wed, Oct 11, 2017 at 11:36 AM, Yugo Nagata <nag...@sraoss.co.jp> wrote:
> >> In the attached patch, only automatically-updatable views that do not have
> >> INSTEAD OF rules or INSTEAD OF triggers are lockable. It is assumed that
> >> those views definition have only one base-relation. When an auto-updatable
> >> view is locked, its base relation is also locked. If the base relation is a
> >> view again, base relations are processed recursively. For locking a view,
> >> the view owner have to have he priviledge to lock the base relation.
> >
> > Why is this the right behavior?
> >
> > I would have expected LOCK TABLE v to lock the view and nothing else.
> >
> > See 
> > http://postgr.es/m/AANLkTi=kupesjhrdevgfbt30au_iyro6zwk+fwwy_...@mail.gmail.com
> > for previous discussion of this topic.
> 
> That's what I would expect as well.. But I may be missing something. I
> am marking the patch as returned with feedback as this has not been
> replied in one month.

I was busy for and I could not work on this patch. After reading the
previous discussion, I still think the behavior of this patch would
be right. So, I would like to reregister to CF 2018-1. Do I need to
create a new entry on CF? or should I change the status to
"Moved to next CF"?

> -- 
> Michael
> 


-- 
Yugo Nagata <nag...@sraoss.co.jp>

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

On Fri, 27 Oct 2017 07:11:14 +0200
Robert Haas <robertmh...@gmail.com> wrote:

> On Wed, Oct 11, 2017 at 11:36 AM, Yugo Nagata <nag...@sraoss.co.jp> wrote:
> > In the attached patch, only automatically-updatable views that do not have
> > INSTEAD OF rules or INSTEAD OF triggers are lockable. It is assumed that
> > those views definition have only one base-relation. When an auto-updatable
> > view is locked, its base relation is also locked. If the base relation is a
> > view again, base relations are processed recursively. For locking a view,
> > the view owner have to have he priviledge to lock the base relation.
> 
> Why is this the right behavior?
> 
> I would have expected LOCK TABLE v to lock the view and nothing else.
> 
> See 
> http://postgr.es/m/AANLkTi=kupesjhrdevgfbt30au_iyro6zwk+fwwy_...@mail.gmail.com
> for previous discussion of this topic.

This discussion is one about 7 years ago when automatically-updatable views
are not supported. Since 9.3, simple views can be updated as well as tables,
so now I think it is reasonable that LOCK TABLE for views locks their base
tables.

If we want to lock only the view, it seems to me that LOCK VIEW syntax is good. 
However, to realize this, changing the syntax to avoid a shift/reduce
conflict will be needed as disucussed in the "LOCK for non-tables" thread.

> 
> -- 
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
> 
> 
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hack...@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


-- 
Yugo Nagata <nag...@sraoss.co.jp>