Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Sat, Nov 11, 2017 at 6:48 AM, Michael Paquier <michael.paqu...@gmail.com>
wrote:
>
> On Sat, Nov 11, 2017 at 12:49 AM, Fabrízio de Royes Mello
> <fabriziome...@gmail.com> wrote:
> > New version attached.
>
> Thanks.
>
> +++ b/src/test/modules/Makefile
>   test_extensions \
> + test_session_hooks \
>   test_parser
> Better if that's in alphabetical order.
>

Fixed.


> That's a nit though, so I am switching the patch as ready for committer.
>

Thank you so much for your review.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 05c5c19..d3156ad 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -169,6 +169,9 @@ static ProcSignalReason RecoveryConflictReason;
 static MemoryContext row_description_context = NULL;
 static StringInfoData row_description_buf;
 
+/* Hook for plugins to get control at start of session */
+session_start_hook_type session_start_hook = NULL;
+
 /* 
  *		decls for routines only used in this file
  * 
@@ -3857,6 +3860,9 @@ PostgresMain(int argc, char *argv[],
 	if (!IsUnderPostmaster)
 		PgStartTime = GetCurrentTimestamp();
 
+	if (session_start_hook)
+		(*session_start_hook) ();
+
 	/*
 	 * POSTGRES main processing loop begins here
 	 *
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 20f1d27..16ec376 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -76,6 +76,8 @@ static bool ThereIsAtLeastOneRole(void);
 static void process_startup_options(Port *port, bool am_superuser);
 static void process_settings(Oid databaseid, Oid roleid);
 
+/* Hook for plugins to get control at end of session */
+session_end_hook_type session_end_hook = NULL;
 
 /*** InitPostgres support ***/
 
@@ -1154,6 +1156,10 @@ ShutdownPostgres(int code, Datum arg)
 	 * them explicitly.
 	 */
 	LockReleaseAll(USER_LOCKMETHOD, true);
+
+	/* Hook at session end */
+	if (session_end_hook)
+		(*session_end_hook) ();
 }
 
 
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index f8c535c..9f05bfb 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -35,6 +35,13 @@ extern PGDLLIMPORT const char *debug_query_string;
 extern int	max_stack_depth;
 extern int	PostAuthDelay;
 
+/* Hook for plugins to get control at start and end of session */
+typedef void (*session_start_hook_type) (void);
+typedef void (*session_end_hook_type) (void);
+
+extern PGDLLIMPORT session_start_hook_type session_start_hook;
+extern PGDLLIMPORT session_end_hook_type session_end_hook;
+
 /* GUC-configurable parameters */
 
 typedef enum
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index b7ed0af..7246552 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -15,6 +15,7 @@ SUBDIRS = \
 		  test_pg_dump \
 		  test_rbtree \
 		  test_rls_hooks \
+		  test_session_hooks \
 		  test_shm_mq \
 		  worker_spi
 
diff --git a/src/test/modules/test_session_hooks/.gitignore b/src/test/modules/test_session_hooks/.gitignore
new file mode 100644
index 000..5dcb3ff
--- /dev/null
+++ b/src/test/modules/test_session_hooks/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_session_hooks/Makefile b/src/test/modules/test_session_hooks/Makefile
new file mode 100644
index 000..c5c3860
--- /dev/null
+++ b/src/test/modules/test_session_hooks/Makefile
@@ -0,0 +1,21 @@
+# src/test/modules/test_session_hooks/Makefile
+
+MODULES = test_session_hooks
+PGFILEDESC = "test_session_hooks - Test session hooks with an extension"
+
+EXTENSION = test_session_hooks
+DATA = test_session_hooks--1.0.sql
+
+REGRESS = test_session_hooks
+REGRESS_OPTS = --temp-config=$(top_srcdir)/src/test/modules/test_session_hooks/session_hooks.conf
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_session_hooks
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_session_hooks/README b/src/test/modules/test_session_hooks/README
new file mode 100644
index 000..9cb4202
--- /dev/null
+++ b/src/test/modules/test_session_hooks/README
@@ -0,0 +1,2 @@
+test_session_hooks is an example of how to use session start and end
+hooks.
diff --git a/src/test/modules/test_session_hooks/expected/test_s

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Thu, Nov 9, 2017 at 9:08 PM, Michael Paquier <michael.paqu...@gmail.com>
wrote:
>
> On Fri, Nov 10, 2017 at 2:32 AM, Fabrízio de Royes Mello
> <fabriziome...@gmail.com> wrote:
> > On Thu, Nov 9, 2017 at 12:09 AM, Michael Paquier <
michael.paqu...@gmail.com>
> > wrote:
> >> +++ b/src/test/modules/test_session_hooks/session_hooks.conf
> >> @@ -0,0 +1 @@
> >> +shared_preload_libraries = 'test_session_hooks'
> >> Don't you think that this should be a GUC? My previous comment
> >> outlined that. I won't fight hard on that point in any case, don't
> >> worry. I just want to make things clear :)
> >
> > Ooops... my fault... fixed!
> >
> > Thanks again!!
>
> +/* GUC variables */
> +static char *session_hook_username = NULL;
> This causes the module to crash if test_session_hooks.username is not
> set. I would recommend just assigning a default value, say "postgres".
>

Fixed.

New version attached.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 05c5c19..d3156ad 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -169,6 +169,9 @@ static ProcSignalReason RecoveryConflictReason;
 static MemoryContext row_description_context = NULL;
 static StringInfoData row_description_buf;
 
+/* Hook for plugins to get control at start of session */
+session_start_hook_type session_start_hook = NULL;
+
 /* 
  *		decls for routines only used in this file
  * 
@@ -3857,6 +3860,9 @@ PostgresMain(int argc, char *argv[],
 	if (!IsUnderPostmaster)
 		PgStartTime = GetCurrentTimestamp();
 
+	if (session_start_hook)
+		(*session_start_hook) ();
+
 	/*
 	 * POSTGRES main processing loop begins here
 	 *
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 20f1d27..16ec376 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -76,6 +76,8 @@ static bool ThereIsAtLeastOneRole(void);
 static void process_startup_options(Port *port, bool am_superuser);
 static void process_settings(Oid databaseid, Oid roleid);
 
+/* Hook for plugins to get control at end of session */
+session_end_hook_type session_end_hook = NULL;
 
 /*** InitPostgres support ***/
 
@@ -1154,6 +1156,10 @@ ShutdownPostgres(int code, Datum arg)
 	 * them explicitly.
 	 */
 	LockReleaseAll(USER_LOCKMETHOD, true);
+
+	/* Hook at session end */
+	if (session_end_hook)
+		(*session_end_hook) ();
 }
 
 
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index f8c535c..9f05bfb 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -35,6 +35,13 @@ extern PGDLLIMPORT const char *debug_query_string;
 extern int	max_stack_depth;
 extern int	PostAuthDelay;
 
+/* Hook for plugins to get control at start and end of session */
+typedef void (*session_start_hook_type) (void);
+typedef void (*session_end_hook_type) (void);
+
+extern PGDLLIMPORT session_start_hook_type session_start_hook;
+extern PGDLLIMPORT session_end_hook_type session_end_hook;
+
 /* GUC-configurable parameters */
 
 typedef enum
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index b7ed0af..a3c8c1e 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -11,6 +11,7 @@ SUBDIRS = \
 		  snapshot_too_old \
 		  test_ddl_deparse \
 		  test_extensions \
+		  test_session_hooks \
 		  test_parser \
 		  test_pg_dump \
 		  test_rbtree \
diff --git a/src/test/modules/test_session_hooks/.gitignore b/src/test/modules/test_session_hooks/.gitignore
new file mode 100644
index 000..5dcb3ff
--- /dev/null
+++ b/src/test/modules/test_session_hooks/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_session_hooks/Makefile b/src/test/modules/test_session_hooks/Makefile
new file mode 100644
index 000..c5c3860
--- /dev/null
+++ b/src/test/modules/test_session_hooks/Makefile
@@ -0,0 +1,21 @@
+# src/test/modules/test_session_hooks/Makefile
+
+MODULES = test_session_hooks
+PGFILEDESC = "test_session_hooks - Test session hooks with an extension"
+
+EXTENSION = test_session_hooks
+DATA = test_session_hooks--1.0.sql
+
+REGRESS = test_session_hooks
+REGRESS_OPTS = --temp-config=$(top_srcdir)/src/test/modules/test_session_hooks/session_hooks.conf
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Thu, Nov 9, 2017 at 12:09 AM, Michael Paquier <michael.paqu...@gmail.com>
wrote:
>
> On Thu, Nov 9, 2017 at 2:42 AM, Fabrízio de Royes Mello
> <fabriziome...@gmail.com> wrote:
> > On Wed, Nov 8, 2017 at 12:47 AM, Michael Paquier <
michael.paqu...@gmail.com>
> > wrote:
> >> - Let's restrict the logging to a role name instead of a database
> >> name, and let's parametrize it with a setting in the temporary
> >> configuration file. Let's not bother about multiple role support with
> >> a list, for the sake of tests and simplicity only defining one role
> >> looks fine to me. Comments in the code should be clear about the
> >> dependency.
> >
> > Makes sense and simplify the test code. Fixed.
>
> +   if (!strcmp(username, "regress_sess_hook_usr2"))
> +   {
> +   const char *dbname;
> [...]
> +++ b/src/test/modules/test_session_hooks/session_hooks.conf
> @@ -0,0 +1 @@
> +shared_preload_libraries = 'test_session_hooks'
> Don't you think that this should be a GUC? My previous comment
> outlined that. I won't fight hard on that point in any case, don't
> worry. I just want to make things clear :)
>

Ooops... my fault... fixed!

Thanks again!!

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 05c5c19..d3156ad 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -169,6 +169,9 @@ static ProcSignalReason RecoveryConflictReason;
 static MemoryContext row_description_context = NULL;
 static StringInfoData row_description_buf;
 
+/* Hook for plugins to get control at start of session */
+session_start_hook_type session_start_hook = NULL;
+
 /* 
  *		decls for routines only used in this file
  * 
@@ -3857,6 +3860,9 @@ PostgresMain(int argc, char *argv[],
 	if (!IsUnderPostmaster)
 		PgStartTime = GetCurrentTimestamp();
 
+	if (session_start_hook)
+		(*session_start_hook) ();
+
 	/*
 	 * POSTGRES main processing loop begins here
 	 *
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 20f1d27..16ec376 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -76,6 +76,8 @@ static bool ThereIsAtLeastOneRole(void);
 static void process_startup_options(Port *port, bool am_superuser);
 static void process_settings(Oid databaseid, Oid roleid);
 
+/* Hook for plugins to get control at end of session */
+session_end_hook_type session_end_hook = NULL;
 
 /*** InitPostgres support ***/
 
@@ -1154,6 +1156,10 @@ ShutdownPostgres(int code, Datum arg)
 	 * them explicitly.
 	 */
 	LockReleaseAll(USER_LOCKMETHOD, true);
+
+	/* Hook at session end */
+	if (session_end_hook)
+		(*session_end_hook) ();
 }
 
 
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index f8c535c..9f05bfb 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -35,6 +35,13 @@ extern PGDLLIMPORT const char *debug_query_string;
 extern int	max_stack_depth;
 extern int	PostAuthDelay;
 
+/* Hook for plugins to get control at start and end of session */
+typedef void (*session_start_hook_type) (void);
+typedef void (*session_end_hook_type) (void);
+
+extern PGDLLIMPORT session_start_hook_type session_start_hook;
+extern PGDLLIMPORT session_end_hook_type session_end_hook;
+
 /* GUC-configurable parameters */
 
 typedef enum
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index b7ed0af..a3c8c1e 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -11,6 +11,7 @@ SUBDIRS = \
 		  snapshot_too_old \
 		  test_ddl_deparse \
 		  test_extensions \
+		  test_session_hooks \
 		  test_parser \
 		  test_pg_dump \
 		  test_rbtree \
diff --git a/src/test/modules/test_session_hooks/.gitignore b/src/test/modules/test_session_hooks/.gitignore
new file mode 100644
index 000..5dcb3ff
--- /dev/null
+++ b/src/test/modules/test_session_hooks/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_session_hooks/Makefile b/src/test/modules/test_session_hooks/Makefile
new file mode 100644
index 000..c5c3860
--- /dev/null
+++ b/src/test/modules/test_session_hooks/Makefile
@@ -0,0 +1,21 @@
+# src/test/modules/test_session_hooks/Makefile
+
+MODULES = test_session_hooks
+PGFILEDESC = "test_session_hooks - Test session hooks with an extension"
+
+EXTENSION = test_session_hooks
+DATA = test_session_hooks--1.0.sql
+
+REGRES

Re: [HACKERS] (spelling) Ensure header of postgresql.auto.conf is consistent

[Fabrízio de Royes Mello]

Em qui, 9 de nov de 2017 às 06:15, Feike Steenbergen <
feikesteenber...@gmail.com> escreveu:

> Attached a patch that ensures the header of postgresql.auto.conf is
> consistent, whether created by initdb or recreated when ALTER SYSTEM
> is issued.
>
> The tiny difference caused some false-positives on our configuration
> management identifying changes, which was enough of an itch for me to
> scratch.


>
Interesting... IMHO this typo should be backpatched to 9.4 when ALTER
SYSTEM was introduced.

Regards,
-- 
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Wed, Nov 8, 2017 at 12:47 AM, Michael Paquier <michael.paqu...@gmail.com>
wrote:
>
> +   /* Hook just normal backends */
> +   if (session_end_hook && MyBackendId != InvalidBackendId)
> +   (*session_end_hook) ();
> I have been wondering about the necessity of this restriction.
> Couldn't it be useful to just let the people implementing the hook do
> any decision-making? Tracking some non-backend shutdowns may be useful
> as well for logging purposes.
>

Also makes sense... I move down this check to hook code.


> The patch is beginning to take shape (I really like the test module
> you are implementing here!), still needs a bit more work.
>

Thanks... and your review is helping a lot!!!


> +CREATE ROLE session_hook_usr1 LOGIN;
> +CREATE ROLE session_hook_usr2 LOGIN;
> Roles created during regression tests should be prefixed with regress_.
>

Fixed.


> +   if (prev_session_start_hook)
> +   prev_session_start_hook();
> +
> +   if (session_start_hook_enabled)
> +   (void) register_session_hook("START");
> Shouldn't both be reversed?
>

Fixed.


> +/* sample sessoin end hook function */
> Typo here.
>

Fixed.


> +CREATE DATABASE session_hook_db;
> [...]
> +   if (!strcmp(dbname, "session_hook_db"))
> +   {
> Creating a database is usually avoided in sql files as those can be
> run on existing servers using installcheck. I am getting that this
> restriction is in place so as it is possible to create an initial
> connection to the server so as the base table to log the activity is
> created. That's a fine assumption to me. Still, I think that the
> following changes should be done:
> - Let's restrict the logging to a role name instead of a database
> name, and let's parametrize it with a setting in the temporary
> configuration file. Let's not bother about multiple role support with
> a list, for the sake of tests and simplicity only defining one role
> looks fine to me. Comments in the code should be clear about the
> dependency.

Makes sense and simplify the test code. Fixed.


> - The GUCs test_session_hooks.start_enabled and
> test_session_hooks.end_enabled are actually redundant with the
> database restriction (well, role restriction per previous comment), so
> let's remove them. Please let's also avoid ALTER SYSTEM calls in tests
> as it would impact existing installations with installcheck.
>

Also simplify the test code. Fixed.


> Impossible to tell committer's feeling about this test suite, but my
> opinion is to keep it as that's a good template example about what can
> be done with those two hooks.
>

+1

Attached new version.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 2c7260e..52a9641 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -169,6 +169,9 @@ static ProcSignalReason RecoveryConflictReason;
 static MemoryContext row_description_context = NULL;
 static StringInfoData row_description_buf;
 
+/* Hook for plugins to get control at start of session */
+session_start_hook_type session_start_hook = NULL;
+
 /* 
  *		decls for routines only used in this file
  * 
@@ -3857,6 +3860,9 @@ PostgresMain(int argc, char *argv[],
 	if (!IsUnderPostmaster)
 		PgStartTime = GetCurrentTimestamp();
 
+	if (session_start_hook)
+		(*session_start_hook) ();
+
 	/*
 	 * POSTGRES main processing loop begins here
 	 *
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 20f1d27..16ec376 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -76,6 +76,8 @@ static bool ThereIsAtLeastOneRole(void);
 static void process_startup_options(Port *port, bool am_superuser);
 static void process_settings(Oid databaseid, Oid roleid);
 
+/* Hook for plugins to get control at end of session */
+session_end_hook_type session_end_hook = NULL;
 
 /*** InitPostgres support ***/
 
@@ -1154,6 +1156,10 @@ ShutdownPostgres(int code, Datum arg)
 	 * them explicitly.
 	 */
 	LockReleaseAll(USER_LOCKMETHOD, true);
+
+	/* Hook at session end */
+	if (session_end_hook)
+		(*session_end_hook) ();
 }
 
 
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index f8c535c..9f05bfb 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -35,6 +35,13 @@ extern PGDLLIMPORT const char *debug_query_string;
 extern int	max_stack_

Re: [HACKERS] Additional logging for VACUUM and ANALYZE

[Fabrízio de Royes Mello]

On Tue, Nov 7, 2017 at 1:35 PM, Bossart, Nathan <bossa...@amazon.com> wrote:
>
> On 11/7/17, 9:13 AM, "Fabrízio Mello" <fabriziome...@gmail.com> wrote:
> >> int save_nestlevel;
> >> +   boolrel_lock;
> >>
> >
> > Just remove the additional tab indentation before rel_lock variable.
>
> I've removed the extra tab in v4.
>

Great. Changed status to ready for commiter.

Regards,


--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Tue, Nov 7, 2017 at 1:15 AM, Michael Paquier <michael.paqu...@gmail.com>
wrote:
>
> On Sun, Nov 5, 2017 at 3:14 AM, Fabrízio de Royes Mello
> <fabriziome...@gmail.com> wrote:
> > On Sat, Nov 4, 2017 at 1:23 AM, Michael Paquier <
michael.paqu...@gmail.com>
> > wrote:
> >> On Fri, Nov 3, 2017 at 1:55 PM, Fabrízio de Royes Mello
> >> <fabriziome...@gmail.com> wrote:
> >> >> Passing the database name and user name does not look much useful to
> >> >> me. You can have access to this data already with CurrentUserId and
> >> >> MyDatabaseId.
> >> >
> >> > This way we don't need to convert oid to names inside hook code.
> >>
> >> Well, arguments of hooks are useful if they are used. Now if I look at
> >> the two examples mainly proposed in this thread, be it in your set of
> >> patches or the possibility to do some SQL transaction, I see nothing
> >> using them. So I'd vote for keeping an interface minimal.
> >>
> >
> > Maybe the attached patch with improved test module can illustrate
better the
> > feature.
>
> I was going to to hack something like that. That's interesting for the
> use case Robert has mentioned.
>
> Well, in the case of the end session hook, those variables are passed
> to the hook by being directly taken from the context from MyProcPort:
> +   (*session_end_hook) (MyProcPort->database_name,
> MyProcPort->user_name);
> In the case of the start hook, those are directly taken from the
> command outer caller, but similarly MyProcPort is already set, so
> those are strings available (your patch does so in the end session
> hook)... Variables in hooks are useful if those are not available
> within the memory context, and refer to a specific state where the
> hook is called. For example, take the password hook, this uses the
> user name and the password because those values are not available
> within the session context. The same stands for other hooks as well.
> Keeping the interface minimal helps in readability and maintenance.
> See for the attached example that can be applied on top of 0003, which
> makes use of the session context, the set regression tests does not
> pass but this shows how I think those hooks had better be shaped.
>

Makes sense... fixed.


> +   (*session_end_hook) (MyProcPort->database_name,
MyProcPort->user_name);
> +
> +   /* Make don't leave any active transactions and/or locks behind */
> +   AbortOutOfAnyTransaction();
> +   LockReleaseAll(USER_LOCKMETHOD, true);
> Let's leave this work to people actually implementing the hook contents.
>

Fixed.

Attached new version. I unify all three patches in just one because this is
a small patch and it's most part is test code.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 2c7260e..52a9641 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -169,6 +169,9 @@ static ProcSignalReason RecoveryConflictReason;
 static MemoryContext row_description_context = NULL;
 static StringInfoData row_description_buf;
 
+/* Hook for plugins to get control at start of session */
+session_start_hook_type session_start_hook = NULL;
+
 /* 
  *		decls for routines only used in this file
  * 
@@ -3857,6 +3860,9 @@ PostgresMain(int argc, char *argv[],
 	if (!IsUnderPostmaster)
 		PgStartTime = GetCurrentTimestamp();
 
+	if (session_start_hook)
+		(*session_start_hook) ();
+
 	/*
 	 * POSTGRES main processing loop begins here
 	 *
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 20f1d27..ffaf51f 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -76,6 +76,8 @@ static bool ThereIsAtLeastOneRole(void);
 static void process_startup_options(Port *port, bool am_superuser);
 static void process_settings(Oid databaseid, Oid roleid);
 
+/* Hook for plugins to get control at end of session */
+session_end_hook_type session_end_hook = NULL;
 
 /*** InitPostgres support ***/
 
@@ -1154,6 +1156,10 @@ ShutdownPostgres(int code, Datum arg)
 	 * them explicitly.
 	 */
 	LockReleaseAll(USER_LOCKMETHOD, true);
+
+	/* Hook just normal backends */
+	if (session_end_hook && MyBackendId != InvalidBackendId)
+		(*session_end_hook) ();
 }
 
 
diff --git a/src/inclu

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Sat, Nov 4, 2017 at 1:23 AM, Michael Paquier <michael.paqu...@gmail.com>
wrote:
>
> On Fri, Nov 3, 2017 at 1:55 PM, Fabrízio de Royes Mello
> <fabriziome...@gmail.com> wrote:
> >> Passing the database name and user name does not look much useful to
> >> me. You can have access to this data already with CurrentUserId and
> >> MyDatabaseId.
> >
> > This way we don't need to convert oid to names inside hook code.
>
> Well, arguments of hooks are useful if they are used. Now if I look at
> the two examples mainly proposed in this thread, be it in your set of
> patches or the possibility to do some SQL transaction, I see nothing
> using them. So I'd vote for keeping an interface minimal.
>

Maybe the attached patch wit improved test module can illustrate better the
feature.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 2c7260e..24346fb 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -169,6 +169,9 @@ static ProcSignalReason RecoveryConflictReason;
 static MemoryContext row_description_context = NULL;
 static StringInfoData row_description_buf;
 
+/* Hook for plugins to get control at start of session */
+session_start_hook_type session_start_hook = NULL;
+
 /* 
  *		decls for routines only used in this file
  * 
@@ -3857,6 +3860,9 @@ PostgresMain(int argc, char *argv[],
 	if (!IsUnderPostmaster)
 		PgStartTime = GetCurrentTimestamp();
 
+	if (session_start_hook)
+		(*session_start_hook) (dbname, username);
+
 	/*
 	 * POSTGRES main processing loop begins here
 	 *
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index f8c535c..d349592 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -35,6 +35,11 @@ extern PGDLLIMPORT const char *debug_query_string;
 extern int	max_stack_depth;
 extern int	PostAuthDelay;
 
+/* Hook for plugins to get control at start of session */
+typedef void (*session_start_hook_type) (const char *dbname,
+		 const char *username);
+extern PGDLLIMPORT session_start_hook_type session_start_hook;
+
 /* GUC-configurable parameters */
 
 typedef enum
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 20f1d27..029eeb4 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -76,6 +76,8 @@ static bool ThereIsAtLeastOneRole(void);
 static void process_startup_options(Port *port, bool am_superuser);
 static void process_settings(Oid databaseid, Oid roleid);
 
+/* Hook for plugins to get control at end of session */
+session_end_hook_type session_end_hook = NULL;
 
 /*** InitPostgres support ***/
 
@@ -1154,6 +1156,16 @@ ShutdownPostgres(int code, Datum arg)
 	 * them explicitly.
 	 */
 	LockReleaseAll(USER_LOCKMETHOD, true);
+
+	/* Hook just normal backends */
+	if (session_end_hook && MyBackendId != InvalidBackendId)
+	{
+		(*session_end_hook) (MyProcPort->database_name, MyProcPort->user_name);
+
+		/* Make don't leave any active transactions and/or locks behind */
+		AbortOutOfAnyTransaction();
+		LockReleaseAll(USER_LOCKMETHOD, true);
+	}
 }
 
 
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index d349592..b7fb8c3 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -35,10 +35,14 @@ extern PGDLLIMPORT const char *debug_query_string;
 extern int	max_stack_depth;
 extern int	PostAuthDelay;
 
-/* Hook for plugins to get control at start of session */
+/* Hook for plugins to get control at start and end of session */
 typedef void (*session_start_hook_type) (const char *dbname,
 		 const char *username);
+typedef void (*session_end_hook_type) (const char *dbname,
+	   const char *username);
+
 extern PGDLLIMPORT session_start_hook_type session_start_hook;
+extern PGDLLIMPORT session_end_hook_type session_end_hook;
 
 /* GUC-configurable parameters */
 
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index b7ed0af..a3c8c1e 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -11,6 +11,7 @@ SUBDIRS = \
 		  snapshot_too_old \
 		  test_ddl_deparse \
 		  test_extensions \
+		  test_session_hooks \
 		  test_parser \
 		  test_pg_dump \
 		  test_rbtree \
diff --git a/src/test/modules/test_session_hooks/.gitignore b/src/test/modules/test_session_hooks/.gitignore
new file mode 100644
index 000..5dcb3ff
--- /dev/null
+++ b/src/test/modules/test_session_hooks/.gitignore
@@ -0,0 +1

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Fri, Nov 3, 2017 at 11:43 AM, Aleksandr Parfenov <
a.parfe...@postgrespro.ru> wrote:
>
> README file in patch 0003 is a copy of README from test_pg_dump module
> without any changes.
>

Thanks, I'll fix it.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Fri, Nov 3, 2017 at 11:19 AM, Michael Paquier <michael.paqu...@gmail.com>
wrote:
>
>  /*
> + * Setup handler to session end hook
> + */
> +if (IsUnderPostmaster)
> +on_proc_exit(do_session_end_hook, 0);
> I think that it would be better to place that in ShutdownPostgres.
> This way it is possible to take actions before any resource is shut
> down.
>

Hmmm... ok but I have some doubt... ShutdownPostgres make sure to abort any
active transaction (AbortOutOfAnyTransaction) and release all locks
(LockReleaseAll). If we hook session end at this point we'll do that after
this two steps and after that run again... something like:

...
/* Make sure we've killed any active transaction */
AbortOutOfAnyTransaction();

/*
 * User locks are not released by transaction end, so be sure to release
 * them explicitly.
 */
LockReleaseAll(USER_LOCKMETHOD, true);

if (session_end_hook) {
(*session_end_hook) (port->database_name, port->user_name);

/* Make sure session end hook doesn't leave trash behind */
AbortOutOfAnyTransaction();
LockReleaseAll(USER_LOCKMETHOD, true);
}
...


> Passing the database name and user name does not look much useful to
> me. You can have access to this data already with CurrentUserId and
> MyDatabaseId.
>

This way we don't need to convert oid to names inside hook code.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Thu, Nov 2, 2017 at 5:42 AM, Aleksandr Parfenov <
a.parfe...@postgrespro.ru> wrote:
>
> The following review has been posted through the commitfest application:
> make installcheck-world:  not tested
> Implements feature:   not tested
> Spec compliant:   not tested
> Documentation:not tested
>
> Hi,
>
> Unfortunately, patches 0001 and 0002 don't apply to current master.
>
> The new status of this patch is: Waiting on Author
>

Thanks for your review. Rebased versions attached.

Regards,


--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 2c7260e..24346fb 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -169,6 +169,9 @@ static ProcSignalReason RecoveryConflictReason;
 static MemoryContext row_description_context = NULL;
 static StringInfoData row_description_buf;
 
+/* Hook for plugins to get control at start of session */
+session_start_hook_type session_start_hook = NULL;
+
 /* 
  *		decls for routines only used in this file
  * 
@@ -3857,6 +3860,9 @@ PostgresMain(int argc, char *argv[],
 	if (!IsUnderPostmaster)
 		PgStartTime = GetCurrentTimestamp();
 
+	if (session_start_hook)
+		(*session_start_hook) (dbname, username);
+
 	/*
 	 * POSTGRES main processing loop begins here
 	 *
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index f8c535c..d349592 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -35,6 +35,11 @@ extern PGDLLIMPORT const char *debug_query_string;
 extern int	max_stack_depth;
 extern int	PostAuthDelay;
 
+/* Hook for plugins to get control at start of session */
+typedef void (*session_start_hook_type) (const char *dbname,
+		 const char *username);
+extern PGDLLIMPORT session_start_hook_type session_start_hook;
+
 /* GUC-configurable parameters */
 
 typedef enum
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 24346fb..0dbbd7b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -169,9 +169,9 @@ static ProcSignalReason RecoveryConflictReason;
 static MemoryContext row_description_context = NULL;
 static StringInfoData row_description_buf;
 
-/* Hook for plugins to get control at start of session */
+/* Hook for plugins to get control at start or end of session */
 session_start_hook_type session_start_hook = NULL;
-
+session_end_hook_type session_end_hook = NULL;
 /* 
  *		decls for routines only used in this file
  * 
@@ -196,6 +196,7 @@ static void drop_unnamed_stmt(void);
 static void log_disconnections(int code, Datum arg);
 static void enable_statement_timeout(void);
 static void disable_statement_timeout(void);
+static void do_session_end_hook(int code, Datum arg);
 
 
 /* 
@@ -3864,6 +3865,12 @@ PostgresMain(int argc, char *argv[],
 		(*session_start_hook) (dbname, username);
 
 	/*
+	 * Setup handler to session end hook
+	 */
+	if (IsUnderPostmaster)
+		on_proc_exit(do_session_end_hook, 0);
+
+	/*
 	 * POSTGRES main processing loop begins here
 	 *
 	 * If an exception is encountered, processing resumes here so we abort the
@@ -4615,3 +4622,15 @@ disable_statement_timeout(void)
 		stmt_timeout_active = false;
 	}
 }
+
+/*
+ * on_proc_exit handler to call session end hook
+ */
+static void
+do_session_end_hook(int code, Datum arg)
+{
+	Port	   *port = MyProcPort;
+
+	if (session_end_hook)
+		(*session_end_hook) (port->database_name, port->user_name);
+}
\ No newline at end of file
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index d349592..b7fb8c3 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -35,10 +35,14 @@ extern PGDLLIMPORT const char *debug_query_string;
 extern int	max_stack_depth;
 extern int	PostAuthDelay;
 
-/* Hook for plugins to get control at start of session */
+/* Hook for plugins to get control at start and end of session */
 typedef void (*session_start_hook_type) (const char *dbname,
 		 const char *username);
+typedef void (*session_end_hook_type) (const char *dbname,
+	   const char *username);
+
 extern PGDLLIMPORT session_start_hook_type session_start_hook;
+extern PGDLLIMPORT session_end_hook_type session_end_hook;
 
 /* GUC-configurable parameters */
 
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
ind

Re: [HACKERS] Disable cross products in postgres

[Fabrízio de Royes Mello]

On Fri, Oct 13, 2017 at 5:08 PM, Robert Haas <robertmh...@gmail.com> wrote:
>
> On Fri, Oct 13, 2017 at 4:06 PM, Gourav Kumar <gourav1...@gmail.com>
wrote:
> > Can you guide me where to look for it?
>
> Search for make_rels_by_clauseless_joins.
>

I wonder if it's possible implement it as an extension using some hook

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] search path security issue?

[Fabrízio de Royes Mello]

On Sun, Oct 8, 2017 at 3:34 PM, Joe Conway <m...@joeconway.com> wrote:
>
> On 10/06/2017 12:52 AM, Magnus Hagander wrote:
> > It would be a nice feature to have in general, like a "basic guc
> > permissions" thing. At least allowing a superuser to prevent exactly
> > this. You could argue the same thing for example for memory parameters
> > and such. We have no permissions at all when it comes to userset gucs
> > today -- and of course, if something should be added about this, it
> > should be done in a way that works for all the userset variables, not
> > just search_path.
>
> +1
>
> I have wished for exactly that more than once before.
>

+1

I have a multi-user and schema database with some user restrictions because
it's a shared environment (i.e: statement_timeout, lock_timeout). But some
"smart" users change to the GUC default and sometimes we suffer with it.
Would be nice to have some kind of guc permissions.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Thu, Oct 5, 2017 at 4:14 PM, Fabrízio de Royes Mello <
fabriziome...@gmail.com> wrote:
>
>
>
> On Tue, Aug 1, 2017 at 4:55 PM, Robert Haas <robertmh...@gmail.com> wrote:
> >
> > On Tue, Aug 1, 2017 at 3:37 PM, Peter Eisentraut
> > <peter.eisentr...@2ndquadrant.com> wrote:
> > > On 7/21/17 12:59, Robert Haas wrote:
> > >> That's an exceedingly-weak argument for rejecting this patch.  The
> > >> fact that you can probably hack around the lack of a hook for most
> > >> reasonable use cases is not an argument for having a hook that does
> > >> what people actually want to do.
> > >
> > > Still nobody has presented a concrete use case so far.
> >
> > I've been asked for this at EDB, too.  Inserting a row into some table
> > on each logon, for example.
> >
> > A quick Google search found 6 previous requests for this feature, some
> > of which describe intended use cases:
> >
> > https://www.postgresql.org/message-id/4ebc6852.5030...@fuzzy.cz (2011)
> >
https://www.postgresql.org/message-id/CAHyXU0wrsYShxmwBxZSGYoiBJa%3DgzEJ17iAeRvaf_vA%2BcoH_qA%40mail.gmail.com
> > (2011)
> >
https://www.postgresql.org/message-id/bay104-w513cf26c0046c9d28747b8d1...@phx.gbl
> > (2009, in Spanish)
> >
https://www.postgresql.org/message-id/758d5e7f0803130227m558d32cdl7159bed00d21f084%40mail.gmail.com
> > (2008)
> >
https://www.postgresql.org/message-id/001a01c48077%240b118e60%240200030a%40gendron.ca
> > (2004)
> >
https://www.postgresql.org/message-id/f96sgcorblsaqv6updv0...@hotmail.com
> > (2000)
> >
>
> Hi all,
>
> I'm sending a new rebased patches and added tests to src/tests/modules as
suggested before.
>

Also added for the next commitfest:

https://commitfest.postgresql.org/15/1318/

Att,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Tue, Aug 1, 2017 at 4:55 PM, Robert Haas <robertmh...@gmail.com> wrote:
>
> On Tue, Aug 1, 2017 at 3:37 PM, Peter Eisentraut
> <peter.eisentr...@2ndquadrant.com> wrote:
> > On 7/21/17 12:59, Robert Haas wrote:
> >> That's an exceedingly-weak argument for rejecting this patch.  The
> >> fact that you can probably hack around the lack of a hook for most
> >> reasonable use cases is not an argument for having a hook that does
> >> what people actually want to do.
> >
> > Still nobody has presented a concrete use case so far.
>
> I've been asked for this at EDB, too.  Inserting a row into some table
> on each logon, for example.
>
> A quick Google search found 6 previous requests for this feature, some
> of which describe intended use cases:
>
> https://www.postgresql.org/message-id/4ebc6852.5030...@fuzzy.cz (2011)
>
https://www.postgresql.org/message-id/CAHyXU0wrsYShxmwBxZSGYoiBJa%3DgzEJ17iAeRvaf_vA%2BcoH_qA%40mail.gmail.com
> (2011)
>
https://www.postgresql.org/message-id/bay104-w513cf26c0046c9d28747b8d1...@phx.gbl
> (2009, in Spanish)
>
https://www.postgresql.org/message-id/758d5e7f0803130227m558d32cdl7159bed00d21f084%40mail.gmail.com
> (2008)
>
https://www.postgresql.org/message-id/001a01c48077%240b118e60%240200030a%40gendron.ca
> (2004)
>
https://www.postgresql.org/message-id/f96sgcorblsaqv6updv0...@hotmail.com
> (2000)
>

Hi all,

I'm sending a new rebased patches and added tests to src/tests/modules as
suggested before.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index c807b00..5c22f2d 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -165,6 +165,9 @@ static bool RecoveryConflictPending = false;
 static bool RecoveryConflictRetryable = true;
 static ProcSignalReason RecoveryConflictReason;
 
+/* Hook for plugins to get control at start of session */
+session_start_hook_type session_start_hook = NULL;
+
 /* 
  *		decls for routines only used in this file
  * 
@@ -3838,6 +3841,9 @@ PostgresMain(int argc, char *argv[],
 	if (!IsUnderPostmaster)
 		PgStartTime = GetCurrentTimestamp();
 
+	if (session_start_hook)
+		(*session_start_hook) (dbname, username);
+
 	/*
 	 * POSTGRES main processing loop begins here
 	 *
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index f8c535c..d349592 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -35,6 +35,11 @@ extern PGDLLIMPORT const char *debug_query_string;
 extern int	max_stack_depth;
 extern int	PostAuthDelay;
 
+/* Hook for plugins to get control at start of session */
+typedef void (*session_start_hook_type) (const char *dbname,
+		 const char *username);
+extern PGDLLIMPORT session_start_hook_type session_start_hook;
+
 /* GUC-configurable parameters */
 
 typedef enum
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 5c22f2d..79f3099 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -165,9 +165,9 @@ static bool RecoveryConflictPending = false;
 static bool RecoveryConflictRetryable = true;
 static ProcSignalReason RecoveryConflictReason;
 
-/* Hook for plugins to get control at start of session */
+/* Hook for plugins to get control at start or end of session */
 session_start_hook_type session_start_hook = NULL;
-
+session_end_hook_type session_end_hook = NULL;
 /* 
  *		decls for routines only used in this file
  * 
@@ -192,6 +192,7 @@ static void drop_unnamed_stmt(void);
 static void log_disconnections(int code, Datum arg);
 static void enable_statement_timeout(void);
 static void disable_statement_timeout(void);
+static void do_session_end_hook(int code, Datum arg);
 
 
 /* 
@@ -3845,6 +3846,12 @@ PostgresMain(int argc, char *argv[],
 		(*session_start_hook) (dbname, username);
 
 	/*
+	 * Setup handler to session end hook
+	 */
+	if (IsUnderPostmaster)
+		on_proc_exit(do_session_end_hook, 0);
+
+	/*
 	 * POSTGRES main processing loop begins here
 	 *
 	 * If an exception is encountered, processing resumes here so we abort the
@@ -4596,3 +4603,15 @@ disable_statement_timeout(void)
 		stmt_timeout_active = false;
 	}
 }
+
+/*
+ * on_proc_exit handler to call session end hook
+ */
+static void
+do_session_end_hook(int code, D

[HACKERS] Segmentation Fault during pg_restore using '--use-list' and '--jobs'

[Fabrízio de Royes Mello]

Hi all,

I'm facing a 'segmentation fault' error using '--use-list' and '--jobs'
options after update to 9.5.8.

We generate a list ignoring some 'TABLE DATA' toc for a selective restore.

See the test case below:

cat < /tmp/test_restore.dump.list

-- rebuild database
cat depCount == 0 &&
 			_tocEntryRestorePass(otherte) == AH->restorePass &&
-			otherte->par_prev != NULL)
+			otherte->par_prev != NULL &&
+			ready_list != NULL)
 		{
 			/* It must be in the pending list, so remove it ... */
 			par_list_remove(otherte);

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

Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

[Fabrízio de Royes Mello]

On Mon, Aug 7, 2017 at 10:43 AM, Robins Tharakan <thara...@gmail.com> wrote:
>
> On 20 July 2017 at 05:14, Robins Tharakan <thara...@gmail.com> wrote:
>>
>> On 20 July 2017 at 05:08, Michael Paquier <michael.paqu...@gmail.com>
wrote:
>>>
>>> On Wed, Jul 19, 2017 at 8:59 PM,
>>> Fabrízio de Royes Mello
>>> > You should add the properly sgml docs for this pg_dumpall change also.
>>>
>>> Tests of pg_dump go to src/bin/pg_dump/t/ and tests for objects in
>>> extensions are in src/test/modules/test_pg_dump, but you just care
>>> about the former with this patch. And if you implement some new tests,
>>> look at the other tests and base your work on that.
>>
>>
>> Thanks Michael /
>> Fabrízio.
>>
>> Updated patch (attached) additionally adds SGML changes for pg_dumpall.
>> (I'll try to work on the tests, but sending this
>> nonetheless
>> ).
>>
>
> Attached is an updated patch (v4) with basic tests for pg_dump /
pg_dumpall.
> (Have zipped it since patch size jumped to ~40kb).
>

The patch applies cleanly to current master and all tests run without
failures.

I also test against all current supported versions (9.2 ... 9.6) and didn't
find any issue.

Changed status to "ready for commiter".

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Fri, Jul 21, 2017 at 12:19 PM, Fabrízio de Royes Mello <
fabriziome...@gmail.com> wrote:
>
>
> On Fri, Jul 21, 2017 at 10:58 AM, Yugo Nagata <nag...@sraoss.co.jp> wrote:
> >
> > On Fri, 21 Jul 2017 10:31:57 -0300
> > Fabrízio de Royes Mello <fabriziome...@gmail.com> wrote:
> >
> > > On Fri, Jul 21, 2017 at 9:35 AM, Yugo Nagata <nag...@sraoss.co.jp>
wrote:
> > > >
> > > > On Fri, 21 Jul 2017 09:53:19 +0800
> > > > Craig Ringer <cr...@2ndquadrant.com> wrote:
> > > >
> > > > > On 21 July 2017 at 08:42, Robert Haas <robertmh...@gmail.com>
wrote:
> > > > >
> > > > > > On Thu, Jul 20, 2017 at 8:27 AM, Fabrízio de Royes Mello
> > > > > > <fabriziome...@gmail.com> wrote:
> > > > > > > I'm not sure your real needs but doesn't it material for
improve
> > > Event
> > > > > > > Triggers???
> > > > > >
> > > > > > I've thought about that, too.  One problem is what to do if the
user
> > > > > > hits ^C while the event trigger procedure is running.  If you
respond
> > > > > > to that by killing the event trigger and letting the user issue
> > > > > > commands, then the event trigger can't be used for security or
> > > > > > auditing purposes because the user might prevent it from doing
> > > > > > whatever it's intended to do with a well-timed interrupt.  If
you
> > > > > > ignore ^C or make it turn into FATAL, then a poorly-crafted
trigger
> > > > > > can lock users out of the database.  Maybe that's OK.  We could
say
> > > > > > "well, if you lock yourself out of the database with your logon
> > > > > > trigger, you get to shut down the database and restart in
single user
> > > > > > mode to recover".
> > > > > >
> > > > > > A hook, as proposed here, is a lot simpler and lacks these
concerns.
> > > > > > Installing code in C into the database is intrinsically risky
> > > > > > anywhere, and not any moreso here than elsewhere.  But it's
also less
> > > > > > accessible to the average user.
> > > > > > <http://www.postgresql.org/mailpref/pgsql-hackers>
> > > > >
> > > > >
> > > > > I'd favour the c hook personally. It's a lot more flexible, and
can be
> > > used
> > > > > by an extension to implement trigger-like behaviour if anyone
wants it,
> > > > > including the extension's choice of error handling decisions.
> > > > >
> > > > > It's also a lot simpler and less intrusive for core. Which is nice
> > > where we
> > > > > don't have something that we don't have anything compelling
destined for
> > > > > core that needs it. (I want to add a bunch of hooks in the logical
> > > > > replication code in pg11 for similar reasons, and so features
like DDL
> > > > > replication can be prototyped as extensions more practically).
> > > > >
> > >
> > > I agree with you both...
> > >
> > >
> > > > > That said, isn't ExecutorStart_hook + ProcessUtility_hook able to
serve
> > > the
> > > > > same job as a session-start hook, albeit at slightly higher
overhead?
> > > You
> > > > > can just test to see if your initial tasks have run yet.
> > > >
> > > > Thank you for your suggestion. Certainly, we can do the similar job
of a
> > > > session-start hook using these existing hooks, although these hooks
are
> > > > triggered when the first query is executed not when the session is
> > > started.
> > > > Now I come to think that an additional hook is not need.
> > > >
> > >
> > > As Nagata said hooks proposed by Craing will happens only when the
first
> > > query is called so I don't know how it works for session start... are
we
> > > missing something?
> >
> > Yes, ExecutorStart_hook + ProcessUtility_hook is not strictly same as
> > session_start hook. If a query is issued a long time since the session
start,
> > the timing the hook happens is largely deviated. It is no problem if we
only
> > want do something once at the session start, but it might be problem if
> > we want to record the timestamp of the session start, for example.
> >
> > >
> > > If we're going to add this 

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Fri, Jul 21, 2017 at 10:58 AM, Yugo Nagata <nag...@sraoss.co.jp> wrote:
>
> On Fri, 21 Jul 2017 10:31:57 -0300
> Fabrízio de Royes Mello <fabriziome...@gmail.com> wrote:
>
> > On Fri, Jul 21, 2017 at 9:35 AM, Yugo Nagata <nag...@sraoss.co.jp>
wrote:
> > >
> > > On Fri, 21 Jul 2017 09:53:19 +0800
> > > Craig Ringer <cr...@2ndquadrant.com> wrote:
> > >
> > > > On 21 July 2017 at 08:42, Robert Haas <robertmh...@gmail.com> wrote:
> > > >
> > > > > On Thu, Jul 20, 2017 at 8:27 AM, Fabrízio de Royes Mello
> > > > > <fabriziome...@gmail.com> wrote:
> > > > > > I'm not sure your real needs but doesn't it material for improve
> > Event
> > > > > > Triggers???
> > > > >
> > > > > I've thought about that, too.  One problem is what to do if the
user
> > > > > hits ^C while the event trigger procedure is running.  If you
respond
> > > > > to that by killing the event trigger and letting the user issue
> > > > > commands, then the event trigger can't be used for security or
> > > > > auditing purposes because the user might prevent it from doing
> > > > > whatever it's intended to do with a well-timed interrupt.  If you
> > > > > ignore ^C or make it turn into FATAL, then a poorly-crafted
trigger
> > > > > can lock users out of the database.  Maybe that's OK.  We could
say
> > > > > "well, if you lock yourself out of the database with your logon
> > > > > trigger, you get to shut down the database and restart in single
user
> > > > > mode to recover".
> > > > >
> > > > > A hook, as proposed here, is a lot simpler and lacks these
concerns.
> > > > > Installing code in C into the database is intrinsically risky
> > > > > anywhere, and not any moreso here than elsewhere.  But it's also
less
> > > > > accessible to the average user.
> > > > > <http://www.postgresql.org/mailpref/pgsql-hackers>
> > > >
> > > >
> > > > I'd favour the c hook personally. It's a lot more flexible, and can
be
> > used
> > > > by an extension to implement trigger-like behaviour if anyone wants
it,
> > > > including the extension's choice of error handling decisions.
> > > >
> > > > It's also a lot simpler and less intrusive for core. Which is nice
> > where we
> > > > don't have something that we don't have anything compelling
destined for
> > > > core that needs it. (I want to add a bunch of hooks in the logical
> > > > replication code in pg11 for similar reasons, and so features like
DDL
> > > > replication can be prototyped as extensions more practically).
> > > >
> >
> > I agree with you both...
> >
> >
> > > > That said, isn't ExecutorStart_hook + ProcessUtility_hook able to
serve
> > the
> > > > same job as a session-start hook, albeit at slightly higher
overhead?
> > You
> > > > can just test to see if your initial tasks have run yet.
> > >
> > > Thank you for your suggestion. Certainly, we can do the similar job
of a
> > > session-start hook using these existing hooks, although these hooks
are
> > > triggered when the first query is executed not when the session is
> > started.
> > > Now I come to think that an additional hook is not need.
> > >
> >
> > As Nagata said hooks proposed by Craing will happens only when the first
> > query is called so I don't know how it works for session start... are we
> > missing something?
>
> Yes, ExecutorStart_hook + ProcessUtility_hook is not strictly same as
> session_start hook. If a query is issued a long time since the session
start,
> the timing the hook happens is largely deviated. It is no problem if we
only
> want do something once at the session start, but it might be problem if
> we want to record the timestamp of the session start, for example.
>
> >
> > If we're going to add this hook what about add a session end hook also?
>
> If someone want the session-start hook, he might want this too.
>

Well if someone wants here are the patches... I just did a minor fix and
cleanup in your previous session_start sample and provide both samples into
the same patch.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziom

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Fri, Jul 21, 2017 at 9:35 AM, Yugo Nagata <nag...@sraoss.co.jp> wrote:
>
> On Fri, 21 Jul 2017 09:53:19 +0800
> Craig Ringer <cr...@2ndquadrant.com> wrote:
>
> > On 21 July 2017 at 08:42, Robert Haas <robertmh...@gmail.com> wrote:
> >
> > > On Thu, Jul 20, 2017 at 8:27 AM, Fabrízio de Royes Mello
> > > <fabriziome...@gmail.com> wrote:
> > > > I'm not sure your real needs but doesn't it material for improve
Event
> > > > Triggers???
> > >
> > > I've thought about that, too.  One problem is what to do if the user
> > > hits ^C while the event trigger procedure is running.  If you respond
> > > to that by killing the event trigger and letting the user issue
> > > commands, then the event trigger can't be used for security or
> > > auditing purposes because the user might prevent it from doing
> > > whatever it's intended to do with a well-timed interrupt.  If you
> > > ignore ^C or make it turn into FATAL, then a poorly-crafted trigger
> > > can lock users out of the database.  Maybe that's OK.  We could say
> > > "well, if you lock yourself out of the database with your logon
> > > trigger, you get to shut down the database and restart in single user
> > > mode to recover".
> > >
> > > A hook, as proposed here, is a lot simpler and lacks these concerns.
> > > Installing code in C into the database is intrinsically risky
> > > anywhere, and not any moreso here than elsewhere.  But it's also less
> > > accessible to the average user.
> > > <http://www.postgresql.org/mailpref/pgsql-hackers>
> >
> >
> > I'd favour the c hook personally. It's a lot more flexible, and can be
used
> > by an extension to implement trigger-like behaviour if anyone wants it,
> > including the extension's choice of error handling decisions.
> >
> > It's also a lot simpler and less intrusive for core. Which is nice
where we
> > don't have something that we don't have anything compelling destined for
> > core that needs it. (I want to add a bunch of hooks in the logical
> > replication code in pg11 for similar reasons, and so features like DDL
> > replication can be prototyped as extensions more practically).
> >

I agree with you both...


> > That said, isn't ExecutorStart_hook + ProcessUtility_hook able to serve
the
> > same job as a session-start hook, albeit at slightly higher overhead?
You
> > can just test to see if your initial tasks have run yet.
>
> Thank you for your suggestion. Certainly, we can do the similar job of a
> session-start hook using these existing hooks, although these hooks are
> triggered when the first query is executed not when the session is
started.
> Now I come to think that an additional hook is not need.
>

As Nagata said hooks proposed by Craing will happens only when the first
query is called so I don't know how it works for session start... are we
missing something?

If we're going to add this hook what about add a session end hook also?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Thu, Jul 20, 2017 at 8:47 AM, Yugo Nagata <nag...@sraoss.co.jp> wrote:
>
> Hi,
>
> Currently, PostgreSQL doen't have a hook triggered at session
> start. Although we already have ClientAuthentication_hook,
> this is triggered during authentication, so we can not
> access the database.
>
> If we have a hook triggerd only once at session start, we may
> do something useful on the session for certain database or user.
>
> For example, one of our clients wanted such feature. He wanted
> to handle encription for specific users, though I don't know
> the detail.
>
> The attached patch (session_start_hook.patch) implements such
> hook.
>
> Another patch, session_start_sample.patch, is a very simple
> example of this hook that changes work_mem values for sessions
> of a specific database.
>
> I would appreciate hearing your opinion on this hook.
>

I'm not sure your real needs but doesn't it material for improve Event
Triggers???

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

[Fabrízio de Royes Mello]

On Wed, Jul 19, 2017 at 3:54 PM, Robins Tharakan <thara...@gmail.com> wrote:
>
>
> On 18 July 2017 at 23:55, David Fetter <da...@fetter.org> wrote:
>>
>> Excellent point about pg_dumpall.  I'll see what I can draft up in the
>> next day or two and report back.
>
>
>
> Hi David,
>
> You may want to consider this patch (attached) which additionally has the
pg_dumpall changes.
> It would be great if you could help with the tests though, am unsure how
to go about them.
>

You should add the properly sgml docs for this pg_dumpall change also.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Directory pg_replslot is not properly cleaned

[Fabrízio de Royes Mello]

On Mon, Jun 19, 2017 at 11:02 AM, Michael Paquier <michael.paqu...@gmail.com>
wrote:
>
> On Mon, Jun 19, 2017 at 10:58 PM, Fabrízio de Royes Mello
> <fabriziome...@gmail.com> wrote:
> > Do you know when the next minor versions will be released? Because
depending
> > of the schedule I'll patch the current customer version because we need
the
> > "pglogical" running stable.
>
> The next round is planned for the 10th of August:
> https://www.postgresql.org/developer/roadmap/
>

I completely forgot this web page ... thanks!

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Directory pg_replslot is not properly cleaned

[Fabrízio de Royes Mello]

On Sun, Jun 18, 2017 at 11:32 PM, Andres Freund <and...@anarazel.de> wrote:
>
> Hi,
>
> On 2017-06-07 15:46:45 -0300, Fabrízio de Royes Mello wrote:
> > > >> Just adding Dimitriy to conversation... previous email I provided
was
> > > >wrong.
> > > >>
> > > >
> > > >Does anyone have some thought about this critical issue?
> > > >
> > >
> > > I plan to look into it over the next few days.
> > >
> >
> > Thanks...
>
> As noted in
>
http://archives.postgresql.org/message-id/20170619023014.qx7zjmnkzy3fwpfl%40alap3.anarazel.de
> I've pushed a fix for this.  Sorry for it taking this long.
>

Don't worry... thank you so much.


> The fix will be included in the next set of minor releases.
>

Do you know when the next minor versions will be released? Because
depending of the schedule I'll patch the current customer version because
we need the "pglogical" running stable.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Directory pg_replslot is not properly cleaned

[Fabrízio de Royes Mello]

On Wed, Jun 7, 2017 at 3:30 PM, Andres Freund <and...@anarazel.de> wrote:
>
>
>
> On June 7, 2017 11:29:28 AM PDT, "Fabrízio de Royes Mello" <
fabriziome...@gmail.com> wrote:
> >On Fri, Jun 2, 2017 at 6:37 PM, Fabrízio de Royes Mello <
> >fabriziome...@gmail.com> wrote:
> >>
> >>
> >> On Fri, Jun 2, 2017 at 6:32 PM, Fabrízio de Royes Mello <
> >fabriziome...@gmail.com> wrote:
> >> >
> >> > Hi all,
> >> >
> >> > This week I faced a out of disk space trouble in 8TB production
> >cluster. During investigation we notice that pg_replslot was the
> >culprit
> >growing more than 1TB in less than 1 (one) hour.
> >> >
> >> > We're using PostgreSQL 9.5.6 with pglogical 1.2.2 replicating to a
> >new
> >9.6 instance and planning the upgrade soon.
> >> >
> >> > What I did? I freed some disk space just to startup PostgreSQL and
> >begin the investigation. During the 'startup recovery' simply the files
> >inside the pg_replslot was tottaly removed. So our trouble with 'out of
> >disk space' disappear. Then the server went up and physical slaves
> >attached
> >normally to master but logical slaves doesn't, staying stalled in
> >'catchup'
> >state.
> >> >
> >> > At this moment the "pg_replslot" directory started growing fast
> >again
> >and forced us to drop the logical replication slot and we lost the
> >logical
> >slave.
> >> >
> >> > Googling awhile I found this thread [1] about a similar issue
> >reported
> >by Dmitriy Sarafannikov and replied by Andres and Álvaro.
> >> >
> >> > I ran the test case provided by Dmitriy [1] against branches:
> >> > - REL9_4_STABLE
> >> > - REL9_5_STABLE
> >> > - REL9_6_STABLE
> >> > - master
> >> >
> >> > After all test the issue remains... and also using the new Logical
> >Replication stuff (CREATE PUB/CREATE SUB). Just after a restart the
> >"pg_replslot" was properly cleaned. The typo in
> >ReorderBufferIterTXNInit
> >complained by Dimitriy was fixed but the issue remains.
> >> >
> >> > Seems no one complain again about this issue and the thread was
> >lost.
> >> >
> >> > The attached is a reworked version of Dimitriy's patch that seems
> >solve
> >the issue. I confess I don't know enough about replication slots code
> >to
> >really know if it's the best solution.
> >> >
> >> > Regards,
> >> >
> >> > [1]
> >
https://www.postgresql.org/message-id/1457621358.355011041%40f382.i.mail.ru
> >> >
> >>
> >> Just adding Dimitriy to conversation... previous email I provided was
> >wrong.
> >>
> >
> >Does anyone have some thought about this critical issue?
> >
>
> I plan to look into it over the next few days.
>

Thanks...


--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Directory pg_replslot is not properly cleaned

[Fabrízio de Royes Mello]

On Fri, Jun 2, 2017 at 6:37 PM, Fabrízio de Royes Mello <
fabriziome...@gmail.com> wrote:
>
>
> On Fri, Jun 2, 2017 at 6:32 PM, Fabrízio de Royes Mello <
fabriziome...@gmail.com> wrote:
> >
> > Hi all,
> >
> > This week I faced a out of disk space trouble in 8TB production
cluster. During investigation we notice that pg_replslot was the culprit
growing more than 1TB in less than 1 (one) hour.
> >
> > We're using PostgreSQL 9.5.6 with pglogical 1.2.2 replicating to a new
9.6 instance and planning the upgrade soon.
> >
> > What I did? I freed some disk space just to startup PostgreSQL and
begin the investigation. During the 'startup recovery' simply the files
inside the pg_replslot was tottaly removed. So our trouble with 'out of
disk space' disappear. Then the server went up and physical slaves attached
normally to master but logical slaves doesn't, staying stalled in 'catchup'
state.
> >
> > At this moment the "pg_replslot" directory started growing fast again
and forced us to drop the logical replication slot and we lost the logical
slave.
> >
> > Googling awhile I found this thread [1] about a similar issue reported
by Dmitriy Sarafannikov and replied by Andres and Álvaro.
> >
> > I ran the test case provided by Dmitriy [1] against branches:
> > - REL9_4_STABLE
> > - REL9_5_STABLE
> > - REL9_6_STABLE
> > - master
> >
> > After all test the issue remains... and also using the new Logical
Replication stuff (CREATE PUB/CREATE SUB). Just after a restart the
"pg_replslot" was properly cleaned. The typo in ReorderBufferIterTXNInit
complained by Dimitriy was fixed but the issue remains.
> >
> > Seems no one complain again about this issue and the thread was lost.
> >
> > The attached is a reworked version of Dimitriy's patch that seems solve
the issue. I confess I don't know enough about replication slots code to
really know if it's the best solution.
> >
> > Regards,
> >
> > [1]
https://www.postgresql.org/message-id/1457621358.355011041%40f382.i.mail.ru
> >
>
> Just adding Dimitriy to conversation... previous email I provided was
wrong.
>

Does anyone have some thought about this critical issue?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Directory pg_replslot is not properly cleaned

[Fabrízio de Royes Mello]

On Fri, Jun 2, 2017 at 6:32 PM, Fabrízio de Royes Mello <
fabriziome...@gmail.com> wrote:
>
> Hi all,
>
> This week I faced a out of disk space trouble in 8TB production cluster.
During investigation we notice that pg_replslot was the culprit growing
more than 1TB in less than 1 (one) hour.
>
> We're using PostgreSQL 9.5.6 with pglogical 1.2.2 replicating to a new
9.6 instance and planning the upgrade soon.
>
> What I did? I freed some disk space just to startup PostgreSQL and begin
the investigation. During the 'startup recovery' simply the files inside
the pg_replslot was tottaly removed. So our trouble with 'out of disk
space' disappear. Then the server went up and physical slaves attached
normally to master but logical slaves doesn't, staying stalled in 'catchup'
state.
>
> At this moment the "pg_replslot" directory started growing fast again and
forced us to drop the logical replication slot and we lost the logical
slave.
>
> Googling awhile I found this thread [1] about a similar issue reported by
Dmitriy Sarafannikov and replied by Andres and Álvaro.
>
> I ran the test case provided by Dmitriy [1] against branches:
> - REL9_4_STABLE
> - REL9_5_STABLE
> - REL9_6_STABLE
> - master
>
> After all test the issue remains... and also using the new Logical
Replication stuff (CREATE PUB/CREATE SUB). Just after a restart the
"pg_replslot" was properly cleaned. The typo in ReorderBufferIterTXNInit
complained by Dimitriy was fixed but the issue remains.
>
> Seems no one complain again about this issue and the thread was lost.
>
> The attached is a reworked version of Dimitriy's patch that seems solve
the issue. I confess I don't know enough about replication slots code to
really know if it's the best solution.
>
> Regards,
>
> [1]
https://www.postgresql.org/message-id/1457621358.355011041%40f382.i.mail.ru
>

Just adding Dimitriy to conversation... previous email I provided was wrong.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

[HACKERS] Directory pg_replslot is not properly cleaned

[Fabrízio de Royes Mello]

Hi all,

This week I faced a out of disk space trouble in 8TB production cluster.
During investigation we notice that pg_replslot was the culprit growing
more than 1TB in less than 1 (one) hour.

We're using PostgreSQL 9.5.6 with pglogical 1.2.2 replicating to a new 9.6
instance and planning the upgrade soon.

What I did? I freed some disk space just to startup PostgreSQL and begin
the investigation. During the 'startup recovery' simply the files inside
the pg_replslot was tottaly removed. So our trouble with 'out of disk
space' disappear. Then the server went up and physical slaves attached
normally to master but logical slaves doesn't, staying stalled in 'catchup'
state.

At this moment the "pg_replslot" directory started growing fast again and
forced us to drop the logical replication slot and we lost the logical
slave.

Googling awhile I found this thread [1] about a similar issue reported by
Dmitriy Sarafannikov and replied by Andres and Álvaro.

I ran the test case provided by Dmitriy [1] against branches:
- REL9_4_STABLE
- REL9_5_STABLE
- REL9_6_STABLE
- master

After all test the issue remains... and also using the new Logical
Replication stuff (CREATE PUB/CREATE SUB). Just after a restart the
"pg_replslot" was properly cleaned. The typo in ReorderBufferIterTXNInit
complained by Dimitriy was fixed but the issue remains.

Seems no one complain again about this issue and the thread was lost.

The attached is a reworked version of Dimitriy's patch that seems solve the
issue. I confess I don't know enough about replication slots code to really
know if it's the best solution.

Regards,

[1]
https://www.postgresql.org/message-id/1457621358.355011041%40f382.i.mail.ru

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 524946a..a538715 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -1142,7 +1142,7 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
 	Assert(found);
 
 	/* remove entries spilled to disk */
-	if (txn->nentries != txn->nentries_mem)
+	if (txn->nentries != txn->nentries_mem || txn->is_known_as_subxact)
 		ReorderBufferRestoreCleanup(rb, txn);
 
 	/* deallocate */

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

Re: [HACKERS] [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM

[Fabrízio de Royes Mello]

On Thu, Mar 23, 2017 at 3:58 PM, Alvaro Herrera <alvhe...@2ndquadrant.com>
wrote:
>
> Copying Fabrízio Mello here, who spent some time trying to work on
> reloptions too.  He may have something to say about the new
> functionality that this patch provides.
>

Thanks Álvaro, I'll look the patch and try to help in some way.

Regards,

--
   Fabrízio de Royes Mello Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE

[Fabrízio de Royes Mello]

On Sat, Jan 28, 2017 at 4:26 PM, Fabrízio de Royes Mello <
fabriziome...@gmail.com> wrote:
>
>
>
> On Fri, Jan 27, 2017 at 8:53 PM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:
> >
> > On 1/26/17 1:20 PM, Fabrízio de Royes Mello wrote:
> > > Ok, but doing in that way the syntax would be:
> > >
> > > COMMENT ON DATABASE CURRENT_DATABASE IS 'comment';
> >
> > Yes, that's right.  We also have ALTER USER CURRENT_USER already.
> >
>
> Ok, but if we make CURRENT_DATABASE reserved wouldn't it lead us to a
conflict with current_database() function?
>

I did what you suggested making CURRENT_DATABASE reserved but I got the
following error during the bootstrap:


The files belonging to this database system will be owned by user
"fabrizio".
This user must also own the server process.

The database cluster will be initialized with locale "en_US.UTF-8".
The default database encoding has accordingly been set to "UTF8".
The default text search configuration will be set to "english".

Data page checksums are disabled.

creating directory /tmp/master5432 ... ok
creating subdirectories ... ok
selecting default max_connections ... 100
selecting default shared_buffers ... 128MB
selecting dynamic shared memory implementation ... posix
creating configuration files ... ok
running bootstrap script ... ok
performing post-bootstrap initialization ... 2017-01-28 16:19:07.994 BRST
[18252] FATAL:  syntax error at or near "current_database" at character 120
2017-01-28 16:19:07.994 BRST [18252] STATEMENT:
/*
 * 5.2
 * INFORMATION_SCHEMA_CATALOG_NAME view
 */

CREATE VIEW information_schema_catalog_name AS
SELECT CAST(current_database() AS sql_identifier) AS catalog_name;

child process exited with exit code 1
initdb: removing data directory "/tmp/master5432"
pg_ctl: directory "/tmp/master5432" does not exist
psql: could not connect to server: No such file or directory
Is the server running locally and accepting
connections on Unix domain socket "/tmp/.s.PGSQL.5432"?
psql: could not connect to server: No such file or directory
Is the server running locally and accepting
connections on Unix domain socket "/tmp/.s.PGSQL.5432"?


If I use CURRENT_CATALOG instead this error doesn't occurs of course...


--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE

[Fabrízio de Royes Mello]

On Fri, Jan 27, 2017 at 8:53 PM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:
>
> On 1/26/17 1:20 PM, Fabrízio de Royes Mello wrote:
> > Ok, but doing in that way the syntax would be:
> >
> > COMMENT ON DATABASE CURRENT_DATABASE IS 'comment';
>
> Yes, that's right.  We also have ALTER USER CURRENT_USER already.
>

Ok, but if we make CURRENT_DATABASE reserved wouldn't it lead us to a
conflict with current_database() function?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE

[Fabrízio de Royes Mello]

On Mon, Jan 9, 2017 at 6:14 PM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:
>
> On 1/9/17 1:34 PM, Robert Haas wrote:
> > On Fri, Jan 6, 2017 at 7:29 PM, Peter Eisentraut
> > <peter.eisentr...@2ndquadrant.com> wrote:
> >> On 1/3/17 11:52 PM, Ashutosh Bapat wrote:
> >>> We will need to make CURRENT_DATABASE a reserved keyword. But I like
> >>> this idea more than COMMENT ON CURRENT DATABASE.
> >>
> >> We already have the reserved key word CURRENT_CATALOG, which is the
> >> standard spelling.  But I wouldn't be bothered if we made
> >> CURRENT_DATABASE somewhat reserved as well.
> >
> > Maybe I'm just lacking in imagination, but what's the argument against
> > spelling it CURRENT DATABASE?
>
> To achieve consistent support for specifying the current database, we
> would need to change the grammar for every command involving databases.
> And it would also set a precedent for similar commands, such as current
> user/role.  Plus support in psql, pg_dump, etc. would get more
complicated.
>
> Instead, it would be simpler to define a grammar symbol like
>
> database_name: ColId | CURRENT_DATABASE
>
> and make a small analogous change in objectaddress.c and you're done.
>
> Compare rolespec in gram.y.
>

Ok, but doing in that way the syntax would be:

COMMENT ON DATABASE CURRENT_DATABASE IS 'comment';

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE

[Fabrízio de Royes Mello]

Hi Ashutosh,

First of all thanks for your review.


On Tue, Jan 3, 2017 at 3:06 AM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:
>
> The patch has white space error
> git apply /mnt/hgfs/tmp/comment_on_current_database_v1.patch
> /mnt/hgfs/tmp/comment_on_current_database_v1.patch:52: trailing
whitespace.
>  * schema-qualified or catalog-qualified.
> warning: 1 line adds whitespace errors.
>

I'll fix.


> The patch compiles clean, regression is clean. I tested auto
> completion of current database, as well pg_dumpall output for comments
> on multiple databases. Those are working fine. Do we want to add a
> testcase for testing the SQL functionality as well as pg_dumpall
> functionality?
>

While looking into the src/test/regress/sql files I didn't find any test
cases for shared objects (databases, roles, tablespaces, procedural
languages, ...). Should we need also add test cases for this kind of
objects?


> Instead of changing get_object_address_unqualified(),
> get_object_address_unqualified() and pg_get_object_address(), should
> we just stick get_database_name(MyDatabaseId) as object name in
> gram.y? That would be much cleaner than special handling of NIL list.
> It looks dubious to set that list as NIL when the target is some
> object. If we do that, we will spend extra cycles in finding database
> id which is ready available as MyDatabaseId, but the code will be
> cleaner. Another possibility is to use objnames to store the database
> name and objargs to store the database id. That means we overload
> objargs, which doesn't looks good either.
>

In the previous thread Alvaro point me out about a possible DDL deparse
inconsistency [1] and because this reason we decide to think better and
rework this patch.

I confess I'm not too happy with this code yet, and thinking better maybe
we should create a new object type called OBJECT_CURRENT_DATABASE to handle
it different than OBJECT_DATABASE. Opinions???


[1]
https://www.postgresql.org/message-id/20150429170406.GI4369%40alvh.no-ip.org

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Commit fest 2017-01 will begin soon!

[Fabrízio de Royes Mello]

Em sáb, 31 de dez de 2016 às 21:52, Michael Paquier <
michael.paqu...@gmail.com> escreveu:

> On Tue, Dec 27, 2016 at 12:41 PM, Michael Paquier
>
>  wrote:
>
> > There are still a couple of days to register patches! So if you don't
>
> > want your fancy feature to be forgotten, please add it in time to the
>
> > CF app. Speaking of which, I am going to have a low bandwidth soon as
>
> > that's a period of National Holidays in Japan for the new year, and I
>
> > don't think I'll be able to mark the CF as in progress AoE time. So if
>
> > somebody could do it for me that would be great :)
>
>
>
> Reminder: just 1 more day. Be careful to have your patches registered!
>
> Hi,

I changed the status to "In Progress".

Regards,

Re: [HACKERS] tab complete regress tests

[Fabrízio de Royes Mello]

Em sáb, 31 de dez de 2016 às 07:11, Pavel Stehule 
escreveu:

> Hi
>
> now the code in tabcomplete become large part of psql. Is there some
> possibility to write regress tests?
>
> I found only this link
>
>
> http://stackoverflow.com/questions/22795767/how-to-test-python-readline-completion
>
> Isn't possible implement it using our current perl TAP infrastructure?

Regards,

[HACKERS] Add support to COMMENT ON CURRENT DATABASE

[Fabrízio de Royes Mello]

Hi all,

The attached patch is reworked from a previous one [1] to better deal with
get_object_address and pg_get_object_address.

Regards,

[1] https://www.postgresql.org/message-id/20150317171836.gc10...@momjian.us

-- 
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/doc/src/sgml/ref/comment.sgml b/doc/src/sgml/ref/comment.sgml
index c1cf587..51b39ab 100644
--- a/doc/src/sgml/ref/comment.sgml
+++ b/doc/src/sgml/ref/comment.sgml
@@ -31,6 +31,7 @@ COMMENT ON
   CONSTRAINT constraint_name ON table_name |
   CONSTRAINT constraint_name ON DOMAIN domain_name |
   CONVERSION object_name |
+  CURRENT DATABASE |
   DATABASE object_name |
   DOMAIN object_name |
   EXTENSION object_name |
@@ -96,6 +97,11 @@ COMMENT ON
   
 
   
+   The CURRENT DATABASE means the comment will be applied to the database
+   where the command is executed.
+  
+
+  
 Comments can be viewed using psql's
 \d family of commands.
 Other user interfaces to retrieve comments can be built atop
@@ -307,6 +313,7 @@ COMMENT ON COLUMN my_table.my_column IS 'Employee ID number';
 COMMENT ON CONVERSION my_conv IS 'Conversion to UTF8';
 COMMENT ON CONSTRAINT bar_col_cons ON bar IS 'Constrains column col';
 COMMENT ON CONSTRAINT dom_col_constr ON DOMAIN dom IS 'Constrains col of domain';
+COMMENT ON CURRENT DATABASE IS 'Current Database Comment';
 COMMENT ON DATABASE my_database IS 'Development Database';
 COMMENT ON DOMAIN my_domain IS 'Email Address Domain';
 COMMENT ON EXTENSION hstore IS 'implements the hstore data type';
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index bb4b080..71bffae 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -621,6 +621,9 @@ static const struct object_type_map
 	{
 		"database", OBJECT_DATABASE
 	},
+	{
+		"current database", OBJECT_DATABASE
+	},
 	/* OCLASS_TBLSPACE */
 	{
 		"tablespace", OBJECT_TABLESPACE
@@ -1053,9 +1056,12 @@ get_object_address_unqualified(ObjectType objtype,
 
 	/*
 	 * The types of names handled by this function are not permitted to be
-	 * schema-qualified or catalog-qualified.
+	 * schema-qualified or catalog-qualified. 
+	 *
+	 * If "CURRENT DATABASE" is used the target object name is NIL so we
+	 * don't need to do this check.
 	 */
-	if (list_length(qualname) != 1)
+	if (list_length(qualname) > 1)
 	{
 		const char *msg;
 
@@ -1101,7 +1107,10 @@ get_object_address_unqualified(ObjectType objtype,
 	}
 
 	/* Format is valid, extract the actual name. */
-	name = strVal(linitial(qualname));
+	if (list_length(qualname) > 0)
+		name = strVal(linitial(qualname));
+	else
+		name = NULL;
 
 	/* Translate name to OID. */
 	switch (objtype)
@@ -1113,7 +1122,10 @@ get_object_address_unqualified(ObjectType objtype,
 			break;
 		case OBJECT_DATABASE:
 			address.classId = DatabaseRelationId;
-			address.objectId = get_database_oid(name, missing_ok);
+			if (name != NULL)
+address.objectId = get_database_oid(name, missing_ok);
+			else
+address.objectId = MyDatabaseId;
 			address.objectSubId = 0;
 			break;
 		case OBJECT_EXTENSION:
@@ -1951,7 +1963,7 @@ pg_get_object_address(PG_FUNCTION_ARGS)
 	else
 	{
 		name = textarray_to_strvaluelist(namearr);
-		if (list_length(name) < 1)
+		if (list_length(name) < 1 && type != OBJECT_DATABASE)
 			ereport(ERROR,
 	(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 	 errmsg("name list length must be at least %d", 1)));
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 08cf5b7..b87aa5a 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -6101,7 +6101,7 @@ opt_restart_seqs:
  *	the object associated with the comment. The form of the statement is:
  *
  *	COMMENT ON [ [ ACCESS METHOD | CONVERSION | COLLATION |
- * DATABASE | DOMAIN |
+ * DATABASE | CURRENT DATABASE | DOMAIN |
  * EXTENSION | EVENT TRIGGER | FOREIGN DATA WRAPPER |
  * FOREIGN TABLE | INDEX | [PROCEDURAL] LANGUAGE |
  * MATERIALIZED VIEW | POLICY | ROLE | SCHEMA | SEQUENCE |
@@ -6135,6 +6135,15 @@ CommentStmt:
 	n->comment = $6;
 	$$ = (Node *) n;
 }
+			| COMMENT ON CURRENT_P DATABASE IS comment_text
+{
+	CommentStmt *n = makeNode(CommentStmt);
+	n->objtype = OBJECT_DATABASE;
+	n->objname = NIL;
+	n->objargs = NIL;
+	n->comment = $6;
+	$$ = (Node *) n;
+}
 			| COMMENT ON TYPE_P Typename IS comment_text
 {
 	CommentStmt *n = makeNode(CommentStmt);
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index e5545b3..2e4746a 100644
--- a/src/bin/pg_dump/pg_du

Re: [HACKERS] proposal: session server side variables

[Fabrízio de Royes Mello]

On Tue, Dec 27, 2016 at 6:55 PM, Pavel Stehule <pavel.steh...@gmail.com>
wrote:
>
>
> 2016-12-27 21:38 GMT+01:00 Fabrízio de Royes Mello <
fabriziome...@gmail.com>:
>>
>>
>> On Fri, Dec 23, 2016 at 4:00 PM, Joe Conway <m...@joeconway.com> wrote:
>> >
>> > >> In the long term, What would be the possible scopes?
>> > >>
>> > >> TRANSACTION, SESSION, PERSISTANT ?
>> > >>
>> > >> Would some scopes orthogonal (eg SHARED between sessions for a USER
in a
>> > >> DATABASE, SHARED at the cluster level?).
>> > >
>> > > I have a plan to support TRANSACTION and SESSION scope. Persistent or
>> > > shared scope needs much more complex rules, and some specialized
extensions
>> > > will be better.
>> >
>> >
>> > I can see where persistent variables would be very useful though.
>> >
>>
>> Can you talk more about your though??
>>
>> I'm thinking about PERSISTENT VARIABLES and maybe they can be the
redesign of our hard coded "reloptions", with the ability to provide users
to create their own customized. If we think more carefully we already have
some persistent variables with specialized context: reloptions (hardcoded),
security labels (local and shared catalog) and comments (local and shared
catalog). I was clear enough?
>
>
> What is difference from table Values(keys varchar primary key, value
text) ? Where is any benefit?
>

IMHO it's a totally different thing because with this approach we'll have:
- track dependency (aka pg_depend)
- different LockLevel for each persistent variable (reloption)
- syscache to improve performance

And It's a way to store metadata about metadata, i.e. property about our
objects. This can be useful to:
- author extensions to store your own properties with each object
- postgres developers to have a simple way to add a new reloption just
adding a new row to bootstrap data

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] proposal: session server side variables

[Fabrízio de Royes Mello]

On Fri, Dec 23, 2016 at 4:00 PM, Joe Conway <m...@joeconway.com> wrote:
>
> >> In the long term, What would be the possible scopes?
> >>
> >> TRANSACTION, SESSION, PERSISTANT ?
> >>
> >> Would some scopes orthogonal (eg SHARED between sessions for a USER in
a
> >> DATABASE, SHARED at the cluster level?).
> >
> > I have a plan to support TRANSACTION and SESSION scope. Persistent or
> > shared scope needs much more complex rules, and some specialized
extensions
> > will be better.
>
>
> I can see where persistent variables would be very useful though.
>

Can you talk more about your though??

I'm thinking about PERSISTENT VARIABLES and maybe they can be the redesign
of our hard coded "reloptions", with the ability to provide users to create
their own customized. If we think more carefully we already have some
persistent variables with specialized context: reloptions (hardcoded),
security labels (local and shared catalog) and comments (local and shared
catalog). I was clear enough?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] new table partitioning breaks \d table to older versions

[Fabrízio de Royes Mello]

On Fri, Dec 9, 2016 at 3:59 PM, Jeff Janes <jeff.ja...@gmail.com> wrote:
>
> Since:
>
> commit f0e44751d7175fa3394da2c8f85e3ceb3cdbfe63
> Author: Robert Haas <rh...@postgresql.org>
> Date:   Wed Dec 7 13:17:43 2016 -0500
>
> Implement table partitioning.
>
> If I use psql compiled from 10devel to connect to a 9.6.1 server, then \d
fails:
>
> psql (10devel-f0e4475, server 9.6.1-16e7c02)
> Type "help" for help.
>
>
> # \d pgbench_accounts
> ERROR:  column c.relpartbound does not exist
> LINE 1: ...ELECT inhparent::pg_catalog.regclass, pg_get_expr(c.relpartb...
>

Looks like they forgot to adjust version check number in describe.c code.

Attched patch fix it.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index f0d955b..a582a37 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1808,7 +1808,7 @@ describeOneTableDetails(const char *schemaname,
 	}
 
 	/* Make footers */
-	if (pset.sversion >= 90600)
+	if (pset.sversion >= 10)
 	{
 		/* Get the partition information  */
 		PGresult   *result;

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

Re: [HACKERS] DROP FUNCTION of multiple functions

[Fabrízio de Royes Mello]

On Tue, Nov 1, 2016 at 2:55 AM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:
>
> Here is a patch series that implements several changes in the internal
> grammar and node representation of function signatures.  They are not
> necessarily meant to be applied separately, but they explain the
> progression of the changes nicely, so I left them like that for review.
>
> The end goal is to make some user-visible changes in DROP FUNCTION and
> possibly other commands that refer to functions.
>
> With these patches, it is now possible to use DROP FUNCTION to drop
> multiple functions at once: DROP FUNCTION func1(), func2(), func3().
> Other DROP commands already supported that, but DROP FUNCTION didn't
> because the internal representation was complicated and couldn't handle
it.
>
> The next step after this would be to allow referring to functions
> without having to supply the arguments, if the name is unique.  This is
> an SQL-standard feature and would be very useful for dealing "business
> logic" functions with 10+ arguments.  The details of that are to be
> worked out, but with the help of the present changes, this would be a
> quite localized change, because the grammar representation is well
> encapsulated.
>

Really nice... just a little about 006, can't we reduce the code bellow?

@@ -823,8 +823,7 @@ get_object_address(ObjectType objtype, List *objname,
List *objargs,
 {
 FuncWithArgs *fwa = (FuncWithArgs *) linitial(objname);
 address.classId = ProcedureRelationId;
-address.objectId =
-LookupAggNameTypeNames(fwa->funcname,
fwa->funcargs, missing_ok);
+address.objectId = LookupAggWithArgs(fwa, missing_ok);
 address.objectSubId = 0;
 break;
 }
@@ -832,8 +831,7 @@ get_object_address(ObjectType objtype, List *objname,
List *objargs,
 {
 FuncWithArgs *fwa = (FuncWithArgs *) linitial(objname);
 address.classId = ProcedureRelationId;
-address.objectId =
-LookupFuncNameTypeNames(fwa->funcname,
fwa->funcargs, missing_ok);
+address.objectId = LookupFuncWithArgs(fwa, missing_ok);
     address.objectSubId = 0;
 break;
 }

Regards,


--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Commit fest 2016-09 is now closed

[Fabrízio de Royes Mello]

On Mon, Oct 3, 2016 at 2:05 AM, Michael Paquier <michael.paqu...@gmail.com>
wrote:
>
> On Mon, Oct 3, 2016 at 2:01 PM, Michael Paquier
> <michael.paqu...@gmail.com> wrote:
> > There was something like 60~70 patches still listed as active in the
> > CF app on Friday my time, so doing the vacuum cleanup has taken some
> > time by marking patches as returned with feedback, rejected (few) and
> > moved to next CF. A couple of things I noted when going through the
> > remaining entries:
> > - a lot of folks have registered to review a patch and did not post a
> > review. Please avoid putting your name on a patch if you can't afford
> > the time to look at a patch.
> > - The rule one patch sent = one patch review of equal difficulty still
works.
> > - Be careful of the status of the patch, and update it correctly. I
> > have noticed a lot of improvements in this area actually!
>
> Forgot something: if you feel that your patch did not receive the
> correct treatment, of course feel free to update its status in the CF
> app, feel free as well to complain at me and/or on this thread if
> necessary. I am sure we will be able to sort things out :)

Hi Michael,

Thank you so much for your help. I was travelling to take a rest after a
long and busy month.

I'll update my statistics and send another email to check the % of the work
was done.

Thanks again,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

[HACKERS] CommitFest 2016-09 status summary

[Fabrízio de Royes Mello]

Hi all,

The current status summary is:

Needs review: 65
Waiting on author: 34
Ready for Commiter: 9
Commited: 88
Moved to next CF: 1
Rejected: 11
Returned with feedback: 11
TOTAL: 219

The current progress is ~51%. We're in the last week of this CF and we
still with about ~30% of patches needing review and ~16% waiting on author
to take an action. So if you have time please consider reviewing at least
one patch. We need your help!

Regards,

-- 
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

[HACKERS] CommitFest 2016-09 status summary

[Fabrízio de Royes Mello]

Hi all,

The current status summary is:

Needs review: 81
Waiting on author: 35
Ready for Commiter: 12
Commited: 78
Moved to next CF: 1
Rejected: 6
Returned with feedback: 6
TOTAL: 219

The current progress is ~39%. The things moving fast but 27 patches still
with no signed reviewer. So if you have time please consider reviewing at
least one patch. We need your help!

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] CommitFest 2016-09

[Fabrízio de Royes Mello]

On Thu, Sep 1, 2016 at 5:11 PM, Fabrízio de Royes Mello <
fabriziome...@gmail.com> wrote:
>
> Please check the "needs reviewer" list
> (https://commitfest.postgresql.org/9/?reviewer=-2) for patches to
> review.  The committers need our help to work.
>

Just to fix the correct link bellow sent in my previous e-mail

https://commitfest.postgresql.org/10/?reviewer=-2

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

[HACKERS] CommitFest 2016-09

[Fabrízio de Royes Mello]

Hi all,

The 2016-09 commitfest is officially in progress, and I'm the manager.

The current status summary is

Needs review: 119
Needs *reviewer*: 63

Please check the "needs reviewer" list
(https://commitfest.postgresql.org/10/?reviewer=-2
<https://commitfest.postgresql.org/9/?reviewer=-2>) for patches to
review.  The committers need our help to work.

If this is you:

Waiting on Author: 15

Then please post an updated patch as soon as possible so it can be
reviewed.  Some of these patches have not seen any activity from the
author in a long time.

The good news is we are already 29% done with the CF:

Committed: 57
Rejected: 6
Returned with Feedback: 4

TOTAL: 219

I'll post a status update on this thread at least once a week and more
often as needed.

Regards,

-- 
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] PostgreSQL 10 kick-off

[Fabrízio de Royes Mello]

On Thu, Sep 1, 2016 at 1:02 PM, Magnus Hagander <mag...@hagander.net> wrote:
>
> On Sep 1, 2016 17:44, "Fabrízio de Royes Mello" <fabriziome...@gmail.com>
wrote:
> >
> >
> >
> > On Thu, Sep 1, 2016 at 8:35 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> > >
> > > Magnus Hagander <mag...@hagander.net> writes:
> > > >> On Thu, Aug 4, 2016 at 12:09 AM, Fabrízio de Royes Mello
> > > >> <fabriziome...@gmail.com> wrote:
> > > >>> If there are no complains about my lack of experience in this
field I
> > > >>> would like do become the next CFM (am I the first brazilian??)
> > >
> > > > Did we make a decision on this? Cf is starting now, are we
appointing
> > > > Fabrizio as the cf manager?
> > >
> > > I didn't hear any other volunteers, so he's it.
> > >
> >
> > Well... I was waiting for an approval from core team... then now I am
the CF manager...
>
> Yup,  clearly you have it now :-)
>

And here we go!!!


> >
> > I'm getting some help from Alvaro to get the admin grants in CommitFest
App to start this party.
>
> Sounds good and don't hesitate to ask any of us if you are wondering
about something or need some help.
>

Alvaro sent a message by IM that I'm granted as admin in commitfest app but
something seams wrong... The administration "link/button" doesn't appear to
me yet... What's the correct way to get this rights?

Thanks in advance!

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] PostgreSQL 10 kick-off

[Fabrízio de Royes Mello]

On Thu, Sep 1, 2016 at 8:35 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>
> Magnus Hagander <mag...@hagander.net> writes:
> >> On Thu, Aug 4, 2016 at 12:09 AM, Fabrízio de Royes Mello
> >> <fabriziome...@gmail.com> wrote:
> >>> If there are no complains about my lack of experience in this field I
> >>> would like do become the next CFM (am I the first brazilian??)
>
> > Did we make a decision on this? Cf is starting now, are we appointing
> > Fabrizio as the cf manager?
>
> I didn't hear any other volunteers, so he's it.
>

Well... I was waiting for an approval from core team... then now I am the
CF manager...

I'm getting some help from Alvaro to get the admin grants in CommitFest App
to start this party.

In a few hours I'm send the emails and officially start the commitfest.

Regards,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Exclude schema during pg_restore

[Fabrízio de Royes Mello]

Em quarta-feira, 31 de agosto de 2016, Michael Banck <mba...@gmx.net>
escreveu:

> Hi,
>
> attached is a small patch that adds an -N option to pg_restore, in order
> to exclude a schema, in addition to -n for the restriction to a schema.
>
> In principle, this could be extended to -t etc., but I think having this
> for schemas would be the most useful with the least effort.
>
> One use case for this would be the need to restore one or more schemas
> first (using -n foo), then all the others (now using -N foo) without (i)
> having to specify them all with -n and (ii) getting errors due to
> already restored objects from the initial schema. While users could be
> told to just ignore the errors/warnings, it would be useful for
> automation when you would like to check for zero errors/warning, for
> example.
>
> I have so far seen two reasons for this use case: (i) Add-ons that are
> not yet an extension and install objects in public (e.g. ESRI ArcGIS),
> requiring the public schema to be present already on restore of user
> schemas and (ii) restoring materialized views that reference objects
> from other schemas; as permissions are restored last, no permissions
> have been granted for those other schemas yet.
>
> Argueably, those reasons could be dealt with as well, but this seems to
> be a generally useful addition to pg_restore, in my opinion.
>
>
Please add it to the next open commitfest.

Regards,



-- 
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] PostgreSQL 10 kick-off

[Fabrízio de Royes Mello]

On Tue, Aug 2, 2016 at 5:44 PM, Alvaro Herrera <alvhe...@2ndquadrant.com>
wrote:
>
> Peter Eisentraut wrote:
> > On 8/1/16 1:08 PM, Fabrízio de Royes Mello wrote:
> > > What knowledge is expected for a CFM? I'm really would like to help
and
> > > also learn more about our development.
> >
> > If you search the wiki for "commitfest" you will find several pages of
> > varying outdatedness that describe the process.
>
> I have cleaned up the wiki a bit, so the authoritative info should be at
> https://wiki.postgresql.org/wiki/CommitFest_Checklist
> Note that what that page describes is not exactly what we do nowadays;
> if someone wants to edit that page to better reflect reality, be my
> guest.  Also see the old (2009) page where an older workflow was
> described:
>
https://wiki.postgresql.org/index.php?title=Running_a_CommitFest=9369
> Perhaps it'd be good to resurrect some of that contents, in modernized
> form, to the current page.
>

Thank you both for the informations.

If there are no complains about my lack of experience in this field I would
like do become the next CFM (am I the first brazilian??)

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] PostgreSQL 10 kick-off

[Fabrízio de Royes Mello]

On Mon, Aug 1, 2016 at 12:25 PM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:
>
> - We need a commit fest manager.  Volunteers, step forward!
>

What knowledge is expected for a CFM? I'm really would like to help and
also learn more about our development.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [COMMITTERS] [HACKERS] Logical decoding

[Fabrízio de Royes Mello]

On Mon, Jul 11, 2016 at 2:13 AM, Michael Paquier <michael.paqu...@gmail.com>
wrote:
>
> On Mon, Jul 11, 2016 at 2:03 PM, Craig Ringer <cr...@2ndquadrant.com>
wrote:
> > On 9 July 2016 at 01:57, Joshua Bay <joshuaba...@gmail.com> wrote:
> >> and where are this code is in the codebase?
> >
> > src/backend/replication/logical/*
> > src/backend/replication/walsender.c
> > src/backend/access/transam/xlogreader.c
> > src/include/access/xlogreader.h
> > src/include/replication/output_plugin.h
> > contrib/test_decoding/
> > doc/src/sgml/logicaldecoding.sgml
>
> Some other references:
> https://wiki.postgresql.org/images/7/77/Fosdem-2015-logical-decoding.pdf
>
> And some other examples of plugins:
> https://github.com/leptonix/decoding-json
> https://github.com/xstevens/decoderbufs
> https://github.com/confluentinc/bottledwater-pg (for kafka)
> https://github.com/michaelpq/pg_plugins/tree/master/decoder_raw (wrote
this one)
>

Nice, also we have wal2json [1].

Regards,


[1] https://github.com/eulerto/wal2json

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Column COMMENTs in CREATE TABLE?

[Fabrízio de Royes Mello]

Em sábado, 2 de julho de 2016, David G. Johnston <david.g.johns...@gmail.com>
escreveu:

> On Sat, Jul 2, 2016 at 12:55 PM, Marko Tiikkaja <ma...@joh.to
> <javascript:_e(%7B%7D,'cvml','ma...@joh.to');>> wrote:
>
>>
>> What I would prefer is something like this:
>>
>> CREATE TABLE foo(
>>   f1 int NOT NULL COMMENT
>> 'the first field',
>>   f2 int NOT NULL COMMENT
>> 'the second field',
>> ...
>> );
>>
>> which would ensure the comments are both next to the field definition
>> they're documenting and that they make it all the way to the database. I
>> looked into the biggest products, and MySQL supports this syntax.  I
>> couldn't find any similar syntax in any other product.
>>
>>
> ​+1 for the idea - though restricting it to columns would not be ideal.
>
>
> CREATE TABLE name
> COMMENT IS
> 'Table Comment Here'
> (
> col1 serial COMMENT IS 'Place comment here'
> )​;
>
>
And what about the other CREATE statements? IMHO if we follow this path
then we should add COMMENT to all CREATE statements and perhaps also to
ALTER. Of course in a set of small patches to make the reviewers life
easier.

Regards,





-- 
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] hstore: add hstore_length function

[Fabrízio de Royes Mello]

On Fri, Jun 3, 2016 at 7:58 AM, Korbin Hoffman <k...@k1.io> wrote:
>
> Hi there-
>
> I've attached a small patch exposing HS_COUNT to the user as
> "hstore_length(hstore)". Documentation, upgrade sql files, and a few
> tests are also included.
>
> Almost every hstore function calls HS_COUNT, but I couldn't determine
> if there was a reason this exposure didn't already exist.
>
> Without this function, I've been converting an hstore into an array
> and then counting it - a more expensive operation (~30-40% slower than
> SELECTing the hstore itself in a few of my tests).
>
> I will add this thread and patch to the next Commitfest.
>

Something goes wrong when applying against master:

$ git apply ~/Downloads/hstore_length-v1.patch
error: contrib/hstore/Makefile: already exists in working directory
error: contrib/hstore/expected/hstore.out: already exists in working
directory
error: contrib/hstore/hstore--1.3.sql: already exists in working directory
error: contrib/hstore/hstore.control: already exists in working directory
error: contrib/hstore/hstore_op.c: already exists in working directory
error: contrib/hstore/sql/hstore.sql: already exists in working directory
error: doc/src/sgml/hstore.sgml: already exists in working directory


Anyway I have some comments:

1) I don't see any reason to add this sort of thing if you're adding a new
function

+ HSTORE_POLLUTE(hstore_length, length);


2) Shouldn't this declaration use 'uint32' instead of 'int' ??

+ int count = HS_COUNT(hs);
+
+ PG_RETURN_INT32(count);

maybe

+  uint32 count = HS_COUNT(hs);
+
+  PG_RETURN_UINT32(count);

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

[Fabrízio de Royes Mello]

On Wed, May 25, 2016 at 3:03 PM, Alvaro Herrera <alvhe...@2ndquadrant.com>
wrote:
>
> Nikolay Shaplov wrote:
> > В письме от 25 мая 2016 13:25:38 Вы написали:
> > > Teodor Sigaev wrote:
> > > > >This all should me moved behind "access method" abstraction...
> > > >
> > > > +1 relopt_kind should be moved in am, at least. Or removed.
> > >
> > > Hm, but we have tablespace options too, so I'm not sure that using AM
as
> > > abstraction level is correct.
> > We will use am for all indexes, and keep all the rest in relopotion.c
for
> > non-indexes. May be divided options catalog into sections one section
for each
> > kind.
>
> That makes sense.  I can review the patch later.
>
> > And as I also would like to use this code for dynamic attoptions later,
I
> > would like to remove relopt_kind at all. Because it spoils live in that
case.
>
> As I remember, Fabrízio (in CC) had a patch for dynamic reloptions, but
> there was some problem with it and we dumped it; see
>
https://www.postgresql.org/message-id/CAFcNs+rqCq1H5eXW-cvdti6T-xo2STEkhjREx=odmakk5ti...@mail.gmail.com
> for previous discussion.
>

Yeah, and it was forked into other long discussion thread [1] that explain
the reasons to patch got rejected.

IMHO we need a way (maybe at SQL level too) to define and manipulate the
reloptions, and this way should cover all database objects. It isn't a
simple patch because we'll need introduce new catalogs, refactor and
rewrite a lot of code... but it should be a better way to do it. Anyway we
can start with your approach and grow it in small pieces. I have a lot of
ideas about it so I'm glad to discuss it if you want.

Regards,

[1]
https://www.postgresql.org/message-id/cafj8prcx_vdcsdbumknhhyr_-n_ctl84_7r+-bj17hckt_m...@mail.gmail.com
[2]
https://www.postgresql.org/message-id/ca+tgmoznfdqt2kotx38yjus3f_avisclxawbmpdwxhyxrg_...@mail.gmail.com

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Does Type Have = Operator?

[Fabrízio de Royes Mello]

On Wed, May 11, 2016 at 9:09 PM, David E. Wheeler <da...@justatheory.com>
wrote:
>
> On May 11, 2016, at 11:01 AM, Fabrízio de Royes Mello <
fabriziome...@gmail.com> wrote:
>
> > I know... but you can do that just in case the current behaviour fail
by cathing it with "begin...exception...", so you'll minimize the looking
for process on catalog.
>
> Yeah, I guess. Honestly 90% of this issue would go away for me if there
was a `json = json` operator. I know there are a couple different ways to
interpret JSON equality, though.
>

Yeah.. it's ugly but you can do something like that:

CREATE OR REPLACE FUNCTION json_equals_to_json(first JSON, second JSON)
RETURNS boolean AS
$$
BEGIN
   RETURN first::TEXT IS NOT DISTINCT FROM second::TEXT;
END
$$
LANGUAGE plpgsql IMMUTABLE;

CREATE OPERATOR = (
   LEFTARG = json,
   RIGHTARG = json,
   PROCEDURE = json_equals_to_json
);


Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Error during restore - dump taken with pg_dumpall -c option

[Fabrízio de Royes Mello]

Em quinta-feira, 12 de maio de 2016, Rushabh Lathia <
rushabh.lat...@gmail.com> escreveu:

>
> On master branch when we do pg_dumpall with -c option, I can see that
> it also dumping the "DROP ROLE pg_signal_backend", which seems wrong.
> Because when you restore the dump, its throwing an error
> "ERROR:  cannot drop role pg_signal_backend because it is required by the
> database system".
>
>
> dumpRoles()::pg_dumpall.c does have logic to not dump "CREATE ROLE"  if the
> rolename starts with "pg_", but similar check is missing into dropRoles()
> function.
>
> PFA patch, to fix the problem in the similar way its been handled into
> dumpRoles().
>
>
Shouldn't this logic be applied just to version >= 9.6? I ask it because
you write a special query filtering rolname !~ '^pg_' and again check it
using strcmp before the drop role output. Is this the expected behavior?


-- 
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Does Type Have = Operator?

[Fabrízio de Royes Mello]

On Wed, May 11, 2016 at 2:07 AM, David E. Wheeler <da...@justatheory.com>
wrote:
>
> On May 10, 2016, at 5:56 PM, Fabrízio de Royes Mello <
fabriziome...@gmail.com> wrote:
>
> > Searching for the operator in pg_operator catalog isn't enought?
>
> Seems like overkill, but will do if there’s nothing else.
>

I know... but you can do that just in case the current behaviour fail by
cathing it with "begin...exception...", so you'll minimize the looking for
process on catalog.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Does Type Have = Operator?

[Fabrízio de Royes Mello]

Em terça-feira, 10 de maio de 2016, David E. Wheeler <da...@justatheory.com>
escreveu:

> Hackers,
>
> pgTAP has a function that compares two values of a given type, which it
> uses for comparing column defaults. It looks like this:
>
> CREATE OR REPLACE FUNCTION _def_is( TEXT, TEXT, anyelement, TEXT )
> RETURNS TEXT AS $$
> DECLARE
> thing text;
> BEGIN
> IF $1 ~ '^[^'']+[(]' THEN
> -- It's a functional default.
> RETURN is( $1, $3, $4 );
> END IF;
>
> EXECUTE 'SELECT is('
>  || COALESCE($1, 'NULL' || '::' || $2) || '::' || $2 || ',
> '
>  || COALESCE(quote_literal($3), 'NULL') || '::' || $2 ||
> ', '
>  || COALESCE(quote_literal($4), 'NULL')
> || ')' INTO thing;
> RETURN thing;
> END;
> $$ LANGUAGE plpgsql;
>
> The is() function does an IS DISTINCT FROM to compare the two values
> passed to it. This has been working pretty well for years, but one place it
> doesn’t work is with JSON values. I get:
>
> LINE 1: SELECT NOT $1 IS DISTINCT FROM $2
>   ^
> HINT:  No operator matches the given name and argument type(s). You
> might need to add explicit type casts.
> QUERY:  SELECT NOT $1 IS DISTINCT FROM $2
>
> This makes sense, of course, and I could fix it by comparing text values
> instead of json values when the values are JSON. But of course the lack of
> a = operator is not limited to JSON. So I’m wondering if there’s an
> interface at the SQL level to tell me whether a type has an = operator?
> That way I could always use text values in those situations.
>
>
Searching for the operator in pg_operator catalog isn't enought?

Regards,



-- 
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] "parallel= " information is not coming in pg_dumpall for create aggregate

[Fabrízio de Royes Mello]

On Thu, Apr 21, 2016 at 12:06 AM, Robert Haas <robertmh...@gmail.com> wrote:
>
> On Mon, Apr 18, 2016 at 10:47 AM, Fabrízio de Royes Mello
> <fabriziome...@gmail.com> wrote:
> >> I checked in PG 9.6 , if we create an aggregate function with saying -
> >> parallel=safe/restricted/unsafe and then take
> >> a pg_dumpall of the entire cluster , "parallel= " is missing from
create
> >> aggregate syntax
> >>
> >> Steps to reproduce -
> >>
> >> .)connect to psql terminal and create an aggregate function
> >>
> >> postgres=# CREATE AGGREGATE unsafe_sum100 (float8)
> >> (
> >> stype = float8,
> >> sfunc = float8pl,
> >> mstype = float8,
> >> msfunc = float8pl,
> >> minvfunc = float8mi,
> >> parallel=safe);
> >> CREATE AGGREGATE
> >>
> >> .)perform pg_dumpall against that cluster
> >>
> >> .)check the content of create aggregate unsafe_sum100 in the file
> >>
> >> "
> >> -
> >> -- Name: unsafe_sum100(double precision); Type: AGGREGATE; Schema:
public;
> >> Owner: centos
> >> --
> >>
> >> CREATE AGGREGATE unsafe_sum100(double precision) (
> >> SFUNC = float8pl,
> >> STYPE = double precision,
> >> MSFUNC = float8pl,
> >> MINVFUNC = float8mi,
> >> MSTYPE = double precision
> >> );
> >>
> >> "
> >
> > You're correct... try the attached patch to fix it.
>
> Nice catch, Tushar.  Thanks for the patch, Fabrízio.  Committed.
>

You're welcome!

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] "parallel= " information is not coming in pg_dumpall for create aggregate

[Fabrízio de Royes Mello]

On Mon, Apr 18, 2016 at 5:30 AM, tushar <tushar.ah...@enterprisedb.com>
wrote:
>
>
> Hi,
>
> I checked in PG 9.6 , if we create an aggregate function with saying -
parallel=safe/restricted/unsafe and then take
> a pg_dumpall of the entire cluster , "parallel= " is missing from create
aggregate syntax
>
> Steps to reproduce -
>
> .)connect to psql terminal and create an aggregate function
>
> postgres=# CREATE AGGREGATE unsafe_sum100 (float8)
> (
> stype = float8,
> sfunc = float8pl,
> mstype = float8,
> msfunc = float8pl,
> minvfunc = float8mi,
> parallel=safe);
> CREATE AGGREGATE
>
> .)perform pg_dumpall against that cluster
>
> .)check the content of create aggregate unsafe_sum100 in the file
>
> "
> -
> -- Name: unsafe_sum100(double precision); Type: AGGREGATE; Schema:
public; Owner: centos
> --
>
> CREATE AGGREGATE unsafe_sum100(double precision) (
> SFUNC = float8pl,
> STYPE = double precision,
> MSFUNC = float8pl,
> MINVFUNC = float8mi,
>     MSTYPE = double precision
> );
>
> "

You're correct... try the attached patch to fix it.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index e1e5bee..396c03d 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -13274,6 +13274,7 @@ dumpAgg(Archive *fout, AggInfo *agginfo)
 	int			i_agginitval;
 	int			i_aggminitval;
 	int			i_convertok;
+	int			i_proparallel;
 	const char *aggtransfn;
 	const char *aggfinalfn;
 	const char *aggcombinefn;
@@ -13295,6 +13296,7 @@ dumpAgg(Archive *fout, AggInfo *agginfo)
 	const char *agginitval;
 	const char *aggminitval;
 	bool		convertok;
+	const char *proparallel;
 
 	/* Skip if not to be dumped */
 	if (!agginfo->aggfn.dobj.dump || dopt->dataOnly)
@@ -13324,7 +13326,8 @@ dumpAgg(Archive *fout, AggInfo *agginfo)
 			"aggmtransspace, aggminitval, "
 			"true AS convertok, "
 			"pg_catalog.pg_get_function_arguments(p.oid) AS funcargs, "
-			"pg_catalog.pg_get_function_identity_arguments(p.oid) AS funciargs "
+			"pg_catalog.pg_get_function_identity_arguments(p.oid) AS funciargs, "
+			"p.proparallel "
 			"FROM pg_catalog.pg_aggregate a, pg_catalog.pg_proc p "
 			"WHERE a.aggfnoid = p.oid "
 			"AND p.oid = '%u'::pg_catalog.oid",
@@ -13472,6 +13475,7 @@ dumpAgg(Archive *fout, AggInfo *agginfo)
 	i_agginitval = PQfnumber(res, "agginitval");
 	i_aggminitval = PQfnumber(res, "aggminitval");
 	i_convertok = PQfnumber(res, "convertok");
+	i_proparallel = PQfnumber(res, "proparallel");
 
 	aggtransfn = PQgetvalue(res, 0, i_aggtransfn);
 	aggfinalfn = PQgetvalue(res, 0, i_aggfinalfn);
@@ -13511,6 +13515,11 @@ dumpAgg(Archive *fout, AggInfo *agginfo)
 
 	aggsig_tag = format_aggregate_signature(agginfo, fout, false);
 
+	if (i_proparallel != -1)
+		proparallel = PQgetvalue(res, 0, PQfnumber(res, "proparallel"));
+	else
+		proparallel = NULL;
+
 	if (!convertok)
 	{
 		write_msg(NULL, "WARNING: aggregate function %s could not be dumped correctly for this database version; ignored\n",
@@ -13622,6 +13631,17 @@ dumpAgg(Archive *fout, AggInfo *agginfo)
 	if (hypothetical)
 		appendPQExpBufferStr(details, ",\nHYPOTHETICAL");
 
+	if (proparallel != NULL && proparallel[0] != PROPARALLEL_UNSAFE)
+	{
+		if (proparallel[0] == PROPARALLEL_SAFE)
+			appendPQExpBufferStr(details, ",\nPARALLEL = safe");
+		else if (proparallel[0] == PROPARALLEL_RESTRICTED)
+			appendPQExpBufferStr(details, ",\nPARALLEL = restricted");
+		else if (proparallel[0] != PROPARALLEL_UNSAFE)
+			exit_horribly(NULL, "unrecognized proparallel value for function \"%s\"\n",
+		  agginfo->aggfn.dobj.name);
+	}
+
 	/*
 	 * DROP must be fully qualified in case same name appears in pg_catalog
 	 */

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

[HACKERS] Unused macros in src/include/access/transam.h

[Fabrízio de Royes Mello]

Hi all,

When dealing with some patch review I've noticed there are two macro is not
used anywhere:

#define TransactionIdStore(xid, dest)   (*(dest) = (xid))
#define StoreInvalidTransactionId(dest) (*(dest) = InvalidTransactionId)

Regards,

-- 
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Sequence Access Method WIP

[Fabrízio de Royes Mello]

On Tue, Mar 29, 2016 at 5:58 PM, Petr Jelinek <p...@2ndquadrant.com> wrote:
>
> On 29/03/16 22:08, Fabrízio de Royes Mello wrote:
>>
>>
>>
>> On Tue, Mar 29, 2016 at 4:59 PM, Petr Jelinek <p...@2ndquadrant.com
>> <mailto:p...@2ndquadrant.com>> wrote:
>>  >
>>  > On 29/03/16 19:46, Fabrízio de Royes Mello wrotez
>>  >>
>>  >>
>>  >>  >
>>  >>  > Hmm I am unable to reproduce this. What OS? Any special configure
>>  >> flags you use?
>>  >>  >
>>  >>
>>  >> In my environment the error remains with your last patches.
>>  >>
>>  >> I didn't use any special.
>>  >>
>>  >> ./configure --prefix=/home/fabrizio/pgsql --enable-cassert
>>  >> --enable-coverage --enable-tap-tests --enable-depend
>>  >> make -s -j8 install
>>  >> make check-world
>>  >>
>>  >>
>>  >> My environment:
>>  >>
>>  >> fabrizio@bagual:/d/postgresql (0002-gapless-seq-petr)
>>  >> $ uname -a
>>  >> Linux bagual 3.13.0-83-generic #127-Ubuntu SMP Fri Mar 11 00:25:37
UTC
>>  >> 2016 x86_64 x86_64 x86_64 GNU/Linux
>>  >>
>>  >> fabrizio@bagual:/d/postgresql (0002-gapless-seq-petr)
>>  >> $ gcc --version
>>  >> gcc (Ubuntu 4.8.4-2ubuntu1~14.04.1) 4.8.4
>>  >> Copyright (C) 2013 Free Software Foundation, Inc.
>>  >> This is free software; see the source for copying conditions.  There
>> is NO
>>  >> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR
>> PURPOSE.
>>  >>
>>  >
>>  > Hmm nothing special indeed, still can't reproduce, I did one blind
>> try for a fix. Can you test with attached?
>>  >
>>
>> Same error... Now I'll leave in a trip but when I arrive I'll try to
>> figure out what happening debugging the code.
>>
>
> Okay, thanks.
>

Hi,

After some debugging seems that the "seqstate->xid" in "wait_for_sequence"
isn't properly initialized or initialized with the wrong value (1258291200).

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Sequence Access Method WIP

[Fabrízio de Royes Mello]

On Tue, Mar 29, 2016 at 4:59 PM, Petr Jelinek <p...@2ndquadrant.com> wrote:
>
> On 29/03/16 19:46, Fabrízio de Royes Mello wrotez
>>
>>
>>  >
>>  > Hmm I am unable to reproduce this. What OS? Any special configure
>> flags you use?
>>  >
>>
>> In my environment the error remains with your last patches.
>>
>> I didn't use any special.
>>
>> ./configure --prefix=/home/fabrizio/pgsql --enable-cassert
>> --enable-coverage --enable-tap-tests --enable-depend
>> make -s -j8 install
>> make check-world
>>
>>
>> My environment:
>>
>> fabrizio@bagual:/d/postgresql (0002-gapless-seq-petr)
>> $ uname -a
>> Linux bagual 3.13.0-83-generic #127-Ubuntu SMP Fri Mar 11 00:25:37 UTC
>> 2016 x86_64 x86_64 x86_64 GNU/Linux
>>
>> fabrizio@bagual:/d/postgresql (0002-gapless-seq-petr)
>> $ gcc --version
>> gcc (Ubuntu 4.8.4-2ubuntu1~14.04.1) 4.8.4
>> Copyright (C) 2013 Free Software Foundation, Inc.
>> This is free software; see the source for copying conditions.  There is
NO
>> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR
PURPOSE.
>>
>
> Hmm nothing special indeed, still can't reproduce, I did one blind try
for a fix. Can you test with attached?
>

Same error... Now I'll leave in a trip but when I arrive I'll try to figure
out what happening debugging the code.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Sequence Access Method WIP

[Fabrízio de Royes Mello]

On Tue, Mar 29, 2016 at 2:26 PM, Petr Jelinek <p...@2ndquadrant.com> wrote:
>
> On 29/03/16 18:50, Fabrízio de Royes Mello wrote:
>>
>>
>>
>> On Tue, Mar 29, 2016 at 12:25 PM, David Steele <da...@pgmasters.net
>> <mailto:da...@pgmasters.net>> wrote:
>>  >
>>  > Hi Petr,
>>  >
>>  > On 3/28/16 3:11 PM, Fabrízio de Royes Mello wrote:
>>  >
>>  >> fabrizio@bagual:~/pgsql
>>  >> $ bin/pg_dump > /tmp/fabrizio.sql
>>  >> pg_dump: [archiver (db)] query failed: ERROR:  column "sequence_name"
>>  >> does not exist
>>  >> LINE 1: SELECT sequence_name, start_value, increment_by, CASE WHEN
i...
>>  >> ^
>>  >> pg_dump: [archiver (db)] query was: SELECT sequence_name,
start_value,
>>  >> increment_by, CASE WHEN increment_by > 0 AND max_value =
>>  >> 9223372036854775807 THEN NULL  WHEN increment_by < 0 AND
max_value =
>>  >> -1 THEN NULL  ELSE max_value END AS max_value, CASE WHEN
>>  >> increment_by > 0 AND min_value = 1 THEN NULL  WHEN increment_by
< 0
>>  >> AND min_value = -9223372036854775807 THEN NULL  ELSE min_value
END
>>  >> AS min_value, cache_value, is_cycled FROM x
>>  >
>>  >
>>  > It looks like a new patch is needed.  I've marked this "waiting on
>> author".
>>  >
>>
>
> Yeah there were some incompatible commits since my last rebase, fixed,
along with the pg_dump bugs..
>

Now all applies without errors, build and "make check" too.



>> But there are other issue in the gapless-seq extension when I ran "make
>> check":
>>
>>   43   CREATE EXTENSION gapless_seq;
>>   44   CREATE SEQUENCE test_gapless USING gapless;
>>   45   SELECT nextval('test_gapless'::regclass);
>>   46 ! ERROR:  could not access status of transaction 1275068416
>>   47 ! DETAIL:  Could not open file "pg_subtrans/4C00": No such file or
>> directory.
>>   48   BEGIN;
>>   49 SELECT nextval('test_gapless'::regclass);
>>   50 ! ERROR:  could not access status of transaction 1275068416
>>   51 ! DETAIL:  Could not open file "pg_subtrans/4C00": No such file or
>> directory.
>>   52 SELECT nextval('test_gapless'::regclass);
>>   53 ! ERROR:  current transaction is aborted, commands ignored until
>> end of transaction block
>>   54 SELECT nextval('test_gapless'::regclass);
>>   55 ! ERROR:  current transaction is aborted, commands ignored until
>> end of transaction block
>>   56   ROLLBACK;
>>   57   SELECT nextval('test_gapless'::regclass);
>>   58 ! ERROR:  could not access status of transaction 1275068416
>>   59 ! DETAIL:  Could not open file "pg_subtrans/4C00": No such file or
>> directory.
>>
>>
>> And I see the same running manually:
>>
>> fabrizio=# create extension gapless_seq;
>> CREATE EXTENSION
>> fabrizio=# create sequence x using gapless;
>> CREATE SEQUENCE
>> fabrizio=# select nextval('x');
>> ERROR:  could not access status of transaction 1258291200
>> DETAIL:  Could not open file "pg_subtrans/4B00": No such file or
directory.
>>
>> Regards,
>>
>
> Hmm I am unable to reproduce this. What OS? Any special configure flags
you use?
>

In my environment the error remains with your last patches.

I didn't use any special.

./configure --prefix=/home/fabrizio/pgsql --enable-cassert
--enable-coverage --enable-tap-tests --enable-depend
make -s -j8 install
make check-world


My environment:

fabrizio@bagual:/d/postgresql (0002-gapless-seq-petr)
$ uname -a
Linux bagual 3.13.0-83-generic #127-Ubuntu SMP Fri Mar 11 00:25:37 UTC 2016
x86_64 x86_64 x86_64 GNU/Linux

fabrizio@bagual:/d/postgresql (0002-gapless-seq-petr)
$ gcc --version
gcc (Ubuntu 4.8.4-2ubuntu1~14.04.1) 4.8.4
Copyright (C) 2013 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.


Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Sequence Access Method WIP

[Fabrízio de Royes Mello]

On Thu, Mar 24, 2016 at 6:12 PM, Petr Jelinek <p...@2ndquadrant.com> wrote:
>
> Hi,
>
> I rebased this on top of the recently committed CREATE ACCESS METHOD.
>

Hi,

I got the above error trying to apply to the current master:

$ git apply /home/fabrizio/Downloads/0001-seqam-2016-03-24.patch
error: patch failed: src/backend/commands/amcmds.c:29
error: src/backend/commands/amcmds.c: patch does not apply


There are a wrong definition at the beginning of the amcmds.c:

 34 <<<<<<< ours
 35 static Oid  lookup_index_am_handler_func(List *handler_name, char
amtype);
 36 static const char *get_am_type_string(char amtype);
 37 ===
 38 static Oid  lookup_am_handler_func(List *handler_name, char amtype);
 39 static char *get_am_type_string(char amtype);
 40 >>>>>>> theirs


After this small fix I can build and ran regress tests without errors.

But running "check-world" I got the error:

make[1]: Leaving directory `/data/postgresql/src/test/regress'
make: Leaving directory `/data/postgresql'
+ pg_dumpall -f /data/postgresql/src/bin/pg_upgrade/tmp_check/dump1.sql
ok 9 - dropuser foobar1 exit code 0
ok 10 - SQL DROP ROLE run: SQL found in server log
ok 11 - fails with nonexistent user
ok
t/080_pg_isready.pl ...
1..10
ok 1 - pg_isready --help exit code 0
ok 2 - pg_isready --help goes to stdout
ok 3 - pg_isready --help nothing to stderr
ok 4 - pg_isready --version exit code 0
ok 5 - pg_isready --version goes to stdout
ok 6 - pg_isready --version nothing to stderr
ok 7 - pg_isready with invalid option nonzero exit code
ok 8 - pg_isready with invalid option prints error message
ok 9 - fails with no server running
pg_dump: [archiver (db)] query failed: ERROR:  column "sequence_name" does
not exist
LINE 1: SELECT sequence_name, start_value, increment_by, CASE WHEN i...
   ^
pg_dump: [archiver (db)] query was: SELECT sequence_name, start_value,
increment_by, CASE WHEN increment_by > 0 AND max_value =
9223372036854775807 THEN NULL  WHEN increment_by < 0 AND max_value = -1
THEN NULL  ELSE max_value END AS max_value, CASE WHEN increment_by > 0
AND min_value = 1 THEN NULL  WHEN increment_by < 0 AND min_value =
-9223372036854775807 THEN NULL  ELSE min_value END AS min_value,
cache_value, is_cycled FROM check_seq
pg_dumpall: pg_dump failed on database "regression", exiting
+ pg_dumpall1_status=1
+ [ /data/postgresql != /data/postgresql ]
+
/data/postgresql/src/bin/pg_upgrade/tmp_check/install//home/fabrizio/pgsql/bin/pg_ctl
-m fast stop
waiting for server to shut down done
server stopped
+ [ -n  ]
+ [ -n  ]
+ [ -n 1 ]
+ echo pg_dumpall of pre-upgrade database cluster failed
pg_dumpall of pre-upgrade database cluster failed
+ exit 1
+ rm -rf /tmp/pg_upgrade_check-3NUa0X
make[2]: *** [check] Error 1
make[2]: Leaving directory `/data/postgresql/src/bin/pg_upgrade'
make[1]: *** [check-pg_upgrade-recurse] Error 2
make[1]: *** Waiting for unfinished jobs



And testing pg_dump itself I got the same error trying to dump a database
that contains a sequence.

fabrizio=# create sequence x;
CREATE SEQUENCE
fabrizio=# \ds
  List of relations
 Schema | Name |   Type   |  Owner
+--+--+--
 public | x| sequence | fabrizio
(1 row)

fabrizio=# \d x
  Sequence "public.x"
Column|   Type|Value
--+---+-
 start_value  | bigint| 1
 increment_by | bigint| 1
 max_value| bigint| 9223372036854775807
 min_value| bigint| 1
 cache_value  | bigint| 1
 is_cycled| boolean   | f
 amstate  | seqam_local_state | (1,f,0)
Access Method: local

fabrizio=# \q

fabrizio@bagual:~/pgsql
$ bin/pg_dump > /tmp/fabrizio.sql
pg_dump: [archiver (db)] query failed: ERROR:  column "sequence_name" does
not exist
LINE 1: SELECT sequence_name, start_value, increment_by, CASE WHEN i...
   ^
pg_dump: [archiver (db)] query was: SELECT sequence_name, start_value,
increment_by, CASE WHEN increment_by > 0 AND max_value =
9223372036854775807 THEN NULL  WHEN increment_by < 0 AND max_value = -1
THEN NULL  ELSE max_value END AS max_value, CASE WHEN increment_by > 0
AND min_value = 1 THEN NULL  WHEN increment_by < 0 AND min_value =
-9223372036854775807 THEN NULL  ELSE min_value END AS min_value,
cache_value, is_cycled FROM x


Toninght I'll review some parts of the code.

Regards,


Att,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] NOT EXIST for PREPARE

[Fabrízio de Royes Mello]

On Tue, Mar 22, 2016 at 6:16 PM, Yury Zhuravlev <u.zhurav...@postgrespro.ru>
wrote:
>
> Fabrízio de Royes Mello wrote:
>>
>>
>> You're correct. Yury please add your patch to the next commitfest.
>
> Done. But I do not have restrictions as part of our PostgresPro distr. I
think this patch will be in production a month.
>

No problem, but as you already know to get a patch reviewed and eventually
accepted by PostgreSQL community you should first add it to a commitfest.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] NOT EXIST for PREPARE

[Fabrízio de Royes Mello]

On Tue, Mar 22, 2016 at 6:04 PM, Yury Zhuravlev <u.zhurav...@postgrespro.ru>
wrote:
>
> I was not sure about the syntax, so this was a prototype. Now, like all
completed yet.
>
>>
>> 1) I think this syntax is wrong... Instead the common should be:
>>
>> PREPARE [IF NOT EXISTS] ...
>
> You right. Done.
>
>
>> 2) All of CINE statements we emit a NOTICE skipping message, so you
should
>> emit a message like it:
>
> Done.
>
>
>> 3) There are no regression tests
>
> Done.
>>
>> 4) There are no docs
>
> Done.
>

I got an error when build this patch.

$ ./configure --prefix=/home/fabrizio/pgsql --enable-cassert
--enable-coverage --enable-tap-tests --enable-depend

...

$ make

...
gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard
-fprofile-arcs -ftest-coverage  -pthread -D_REENTRANT -D_THREAD_SAFE
-D_POSIX_PTHREAD_SEMANTICS -DECPG_COMPILE -I../include
-I../../../../src/interfaces/ecpg/include -I. -I. -DMAJOR_VERSION=4
-DMINOR_VERSION=12 -DPATCHLEVEL=0 -I../../../../src/include -D_GNU_SOURCE
-c -o preproc.o preproc.c -MMD -MP -MF .deps/preproc.Po
preproc.y: In function ‘base_yyparse’:
preproc.y:8654:15: error: incompatible types when assigning to type ‘struct
prep’ from type ‘char *’
  $$ = cat_str(5,mm_strdup("prepare if not
exists"),$5,$6,mm_strdup("as"),$8);
   ^
make[4]: *** [preproc.o] Error 1
make[4]: Leaving directory
`/data/fabrizio/Dropbox/dev/postgresql/src/interfaces/ecpg/preproc'
make[3]: *** [all-preproc-recurse] Error 2
make[3]: Leaving directory
`/data/fabrizio/Dropbox/dev/postgresql/src/interfaces/ecpg'
make[2]: *** [all-ecpg-recurse] Error 2
make[2]: Leaving directory
`/data/fabrizio/Dropbox/dev/postgresql/src/interfaces'
make[1]: *** [all-interfaces-recurse] Error 2
make[1]: Leaving directory `/data/fabrizio/Dropbox/dev/postgresql/src'
make: *** [all-src-recurse] Error 2



I also didin't see no psql tab-complete code.


Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] NOT EXIST for PREPARE

[Fabrízio de Royes Mello]

On Tue, Mar 22, 2016 at 2:19 PM, Yury Zhuravlev <u.zhurav...@postgrespro.ru>
wrote:
>
> Yury Zhuravlev wrote:
>>>
>>> You already have a patch? If yes I'm glad to review it.
>>>
>> Please. Patch in attachment.
>
>
> Fix bug, forgot change attr number in parser. And, I forgot example:
> PREPARE usrrptplan (int) IF NOT EXISTS AS
>SELECT * FROM pg_operator;
> PREPARE
>
> New patch in attachment.
>

I'll review it soon... but just a few comments:

1) I think this syntax is wrong... Instead the common should be:

PREPARE [IF NOT EXISTS] ...


2) All of CINE statements we emit a NOTICE skipping message, so you should
emit a message like it:

   ereport(NOTICE,
   (errcode(ERRCODE_DUPLICATE_PSTATEMENT),
errmsg("prepared statement \"%s\" already exists,
skipping",
   stmt->name)));


3) There are no regression tests

4) There are no docs


Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] NOT EXIST for PREPARE

[Fabrízio de Royes Mello]

On Tue, Mar 22, 2016 at 2:32 PM, Alvaro Herrera <alvhe...@2ndquadrant.com>
wrote:
>
> Yury Zhuravlev wrote:
> > Fabrízio de Royes Mello wrote:
> > >You already have a patch? If yes I'm glad to review it.
> >
> > If the community is not against it, I'll do it quickly. Changing the
syntax
> > is the risk. In addition, we have already missed 9.6.
>
> Also we're in the middle of a commitfest, and it would be polite to
> review the patches that have been listed in it for almost two months
> now.  We shouldn't distract reviewing power towards new patches at this
> point.
>

You're correct. Yury please add your patch to the next commitfest.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] NOT EXIST for PREPARE

[Fabrízio de Royes Mello]

On Tue, Mar 22, 2016 at 11:50 AM, Stephen Frost <sfr...@snowman.net> wrote:
>
> I agree that PREPARE IF NOT EXISTS would be nice to have, but only if we
> can keep it fast somehow, which is the part that makes me wonder a bit.
>

Skip error if already exists when catched in src/backend/commands/prepare.c
isn't enough?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] NOT EXIST for PREPARE

[Fabrízio de Royes Mello]

On Tue, Mar 22, 2016 at 10:01 AM, Andres Freund <and...@anarazel.de> wrote:
>
> Hi,
>
>
> On 2016-03-22 12:41:43 +0300, Yury Zhuravlev wrote:
> > Do I understand correctly the only way know availability PREPARE it will
> > appeal to pg_prepared_statements?
> > I think this is not a good practice. In some cases, we may not be aware
of
> > the PREPARE made (pgpool). Moreover, it seems popular question in the
> > Internet:
http://stackoverflow.com/questions/1193020/php-postgresql-check-if-a-prepared-statement-already-exists
> >
> > What do you think about adding NOT EXIST functionality to PREPARE?
>
> Not very much. If you're not in in control of the prepared statements, you
> can't be sure it's not an entirely different statement. So NOT EXISTS
> doesn't really buy you anything, you'd still need to compare the
> statement somehow.
>

You're correct, but IMHO it should be used when you have control of
prepared statement... isn't it analogous to CREATE TABLE IF NOT EXISTS??

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] NOT EXIST for PREPARE

[Fabrízio de Royes Mello]

Em terça-feira, 22 de março de 2016, Yury Zhuravlev <
u.zhurav...@postgrespro.ru> escreveu:

> Fabrízio de Royes Mello wrote:
>
>> I think you meant IF NOT EXISTS, right?
>>
> Thanks, you right.
>
>
You already have a patch? If yes I'm glad to review it.

Regards,


-- 
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] NOT EXIST for PREPARE

[Fabrízio de Royes Mello]

Em terça-feira, 22 de março de 2016, Yury Zhuravlev <
u.zhurav...@postgrespro.ru> escreveu:

> Hello hackers.
>
> Do I understand correctly the only way know availability PREPARE it will
> appeal to pg_prepared_statements?
> I think this is not a good practice. In some cases, we may not be aware of
> the PREPARE made (pgpool). Moreover, it seems popular question in the
> Internet:
> http://stackoverflow.com/questions/1193020/php-postgresql-check-if-a-prepared-statement-already-exists
>
>
+1


> What do you think about adding NOT EXIST functionality to PREPARE?
>

>
I think you meant IF NOT EXISTS, right?

Regards,


-- 
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

[HACKERS] gzclose don't set properly the errno in pg_restore

[Fabrízio de Royes Mello]

Hi all,

I'm facing with a strange error message during a failed pg_restore:

pg_restore: processing data for table "historicopesquisaitem"
pg_restore: [directory archiver] could not close data file: Success

I got this error trying to restore from a directory dump file.

Looking at the pg_restore code seems that "gzclose" didn't set the 'errno'
because I got a Z_BUF_ERROR [1] from "cfclose" return and the "errno" still
0 (zero).

412 if (cfclose(cfp) !=0)
413 exit_horribly(modulename, "could not close data file: %s\n",
414   strerror(errno));

Should we set the errno using the gzclose return code or just add some
check to "strerror" the "errno" or the "gzclose return code" if they are
different?

Thoughts?


[1] http://www.zlib.net/manual.html

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Publish autovacuum informations

[Fabrízio de Royes Mello]

On Tue, Mar 1, 2016 at 8:44 AM, Julien Rouhaud <julien.rouh...@dalibo.com>
wrote:
>
> On 01/03/2016 07:50, Michael Paquier wrote:
> > On Tue, Mar 1, 2016 at 4:38 AM, Julien Rouhaud
> > <julien.rouh...@dalibo.com> wrote:
> >> On 29/02/2016 20:20, Fabrízio de Royes Mello wrote:
> >>>
> >>> On Mon, Feb 29, 2016 at 3:04 PM, Julien Rouhaud
> >>> <julien.rouh...@dalibo.com <mailto:julien.rouh...@dalibo.com>> wrote:
> >>>>
> >>>> On 04/06/2015 22:10, Guillaume Lelarge wrote:
> >>>>> 2015-01-05 17:44 GMT+01:00 Guillaume Lelarge <guilla...@lelarge.info
> >>> <mailto:guilla...@lelarge.info>
> >>>>> <mailto:guilla...@lelarge.info <mailto:guilla...@lelarge.info>>>:
> >>>>>
> >>>>> 2015-01-05 17:40 GMT+01:00 Robert Haas <robertmh...@gmail.com
> >>> <mailto:robertmh...@gmail.com>
> >>>>> <mailto:robertmh...@gmail.com <mailto:robertmh...@gmail.com>>>:
> >>>>>
> >>>>> On Wed, Dec 31, 2014 at 12:46 PM, Tom Lane
> >>> <t...@sss.pgh.pa.us <mailto:t...@sss.pgh.pa.us>
> >>>>> <mailto:t...@sss.pgh.pa.us <mailto:t...@sss.pgh.pa.us>>>
wrote:
> >>>>> > I'd be all right with putting the data structure
> >>> declarations in a file
> >>>>> > named something like autovacuum_private.h, especially if
> >>> it carried an
> >>>>> > annotation that "if you depend on this, don't be surprised
> >>> if we break
> >>>>> > your code in future".
> >>>>>
> >>>>> Works for me.  I am not in general surprised when we do
> >>> things that
> >>>>> break my code, or anyway, the code that I'm responsible for
> >>>>> maintaining.  But I think it makes sense to segregate this
> >>> into a
> >>>>> separate header file so that we are clear that it is only
> >>>>> exposed for
> >>>>> the benefit of extension authors, not so that other things
in
> >>>>> the core
> >>>>> system can touch it.
> >>>>>
> >>>>>
> >>>>> I'm fine with that too. I'll try to find some time to work on
that.
> >>>>>
> >>>>>
> >>>>> So I took a look at this this week. I discovered, with the help of a
> >>>>> coworker, that I can already use the AutoVacuumShmem pointer and
read
> >>>>> the struct. Unfortunately, it doesn't give me as much details as I
would
> >>>>> have liked. The list of databases and tables aren't in shared
memory.
> >>>>> They are local to the process that uses them. Putting them in shared
> >>>>> memory (if at all possible) would imply a much bigger patch than I
was
> >>>>> willing to write right now.
> >>>>>
> >>>>> Thanks anyway for the help.
> >>>>>
> >>>>>
> >>>>
> >>>> Sorry to revive such an old thread.
> >>>>
> >>>> I think some hooks in the autovacuum could be enough to have good
> >>>> insight without exposing private structure.
> >
> > Instead of introducing 4 new hooks, which do not represent a general
> > use actually, why don't you expose a portion of this information in
> > shared memory as mentioned upthread? This sounds like a good approach
> > to me. Your extension could then scan them as needed and put that on
> > view or a function. This information is now private in the autovacuum
> > processes, exposing them would allow plugin authors to do a bunch of
> > fancy things I think, in a more flexible way than those hooks. And
> > there is no need to add more hooks should the structure of the
> > autovacuum code change for a reason or another in the future.
> >
>
> I thought exposing private structures could be a blocking issue.  I
> tried to see what could be done using hooks, and one thing I like is
> that we can compute the process time of each relation, or even aggregate
> some statistics.  Having the vacuum time is something that we can
> actually only obtain by setting log_autovacuum_min_duration and parsing
> the logs, and I don't think it would be possible to do this by just
> exposing current private structure.
>

We understood (IMHO is an interesting idea) but as Michael said hooks is
for a general purpose. So can you demonstrate other use cases for this new
hooks?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Publish autovacuum informations

[Fabrízio de Royes Mello]

On Mon, Feb 29, 2016 at 3:04 PM, Julien Rouhaud <julien.rouh...@dalibo.com>
wrote:
>
> On 04/06/2015 22:10, Guillaume Lelarge wrote:
> > 2015-01-05 17:44 GMT+01:00 Guillaume Lelarge <guilla...@lelarge.info
> > <mailto:guilla...@lelarge.info>>:
> >
> > 2015-01-05 17:40 GMT+01:00 Robert Haas <robertmh...@gmail.com
> > <mailto:robertmh...@gmail.com>>:
> >
> > On Wed, Dec 31, 2014 at 12:46 PM, Tom Lane <t...@sss.pgh.pa.us
> > <mailto:t...@sss.pgh.pa.us>> wrote:
> > > I'd be all right with putting the data structure declarations
in a file
> > > named something like autovacuum_private.h, especially if it
carried an
> > > annotation that "if you depend on this, don't be surprised if
we break
> > > your code in future".
> >
> > Works for me.  I am not in general surprised when we do things
that
> > break my code, or anyway, the code that I'm responsible for
> > maintaining.  But I think it makes sense to segregate this into
a
> > separate header file so that we are clear that it is only
> > exposed for
> > the benefit of extension authors, not so that other things in
> > the core
> > system can touch it.
> >
> >
> > I'm fine with that too. I'll try to find some time to work on that.
> >
> >
> > So I took a look at this this week. I discovered, with the help of a
> > coworker, that I can already use the AutoVacuumShmem pointer and read
> > the struct. Unfortunately, it doesn't give me as much details as I would
> > have liked. The list of databases and tables aren't in shared memory.
> > They are local to the process that uses them. Putting them in shared
> > memory (if at all possible) would imply a much bigger patch than I was
> > willing to write right now.
> >
> > Thanks anyway for the help.
> >
> >
>
> Sorry to revive such an old thread.
>
> I think some hooks in the autovacuum could be enough to have good
> insight without exposing private structure.
>

Interesting idea...


> Please find attached a patch that adds some hooks to the autovacuum, and
> as an example a quick proof of concept extension that use them and allow
> to see what are the autovacuum worker todo list, skipped tables and so on.
>
> I'm not really sure about which information should be provided, so I'm
> open to any suggestion to improve this.
>

I have a look at the patch and it's compile without warning and without
regression.

But something goes wrong when installing the extension:

fabrizio@bagual:~/Downloads/pg_stat_autovacuum
$ pg_config
BINDIR = /data/home/fabrizio/pgsql/bin
DOCDIR = /data/home/fabrizio/pgsql/share/doc
HTMLDIR = /data/home/fabrizio/pgsql/share/doc
INCLUDEDIR = /data/home/fabrizio/pgsql/include
PKGINCLUDEDIR = /data/home/fabrizio/pgsql/include
INCLUDEDIR-SERVER = /data/home/fabrizio/pgsql/include/server
LIBDIR = /data/home/fabrizio/pgsql/lib
PKGLIBDIR = /data/home/fabrizio/pgsql/lib
LOCALEDIR = /data/home/fabrizio/pgsql/share/locale
MANDIR = /data/home/fabrizio/pgsql/share/man
SHAREDIR = /data/home/fabrizio/pgsql/share
SYSCONFDIR = /data/home/fabrizio/pgsql/etc
PGXS = /data/home/fabrizio/pgsql/lib/pgxs/src/makefiles/pgxs.mk
CONFIGURE = '--prefix=/home/fabrizio/pgsql' '--enable-cassert'
'--enable-coverage' '--enable-tap-tests' '--enable-depend'
CC = gcc
CPPFLAGS = -DFRONTEND -D_GNU_SOURCE
CFLAGS = -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard
-fprofile-arcs -ftest-coverage
CFLAGS_SL = -fpic
LDFLAGS = -L../../src/common -Wl,--as-needed
-Wl,-rpath,'/home/fabrizio/pgsql/lib',--enable-new-dtags
LDFLAGS_EX =
LDFLAGS_SL =
LIBS = -lpgcommon -lpgport -lz -lreadline -lrt -lcrypt -ldl -lm
VERSION = PostgreSQL 9.6devel

fabrizio@bagual:~/Downloads/pg_stat_autovacuum
$ make USE_PGXS=1 install
/bin/mkdir -p '/data/home/fabrizio/pgsql/lib'
/bin/mkdir -p '/data/home/fabrizio/pgsql/share/extension'
/bin/mkdir -p '/data/home/fabrizio/pgsql/share/extension'
/bin/mkdir -p '/data/home/fabrizio/pgsql/share/doc/extension'
/usr/bin/install -c -m 755  pg_stat_autovacuum.so
'/data/home/fabrizio/pgsql/lib/pg_stat_autovacuum.so'
/usr/bin/install -c -m 644 .//pg_stat_autovacuum.control
'/data/home/fabrizio/pgsql/share/extension/'
/usr/bin/install -c -m 644 .//pg_stat_autovacuum--0.0.1.sql
'/data/home/fabrizio/pgsql/share/extension/'
/usr/bin/install -c -m 644  '/data/home/fabrizio/pgsql/share/doc/extension/'
/usr/bin/install: missing destination file operand after
‘/data/home/fabrizio/pgsql/share/doc/extension/’
Try '/usr/bin/install --help' for more information.
make: *** [install] Error 1

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Reduce lock levels others reloptions in ALTER TABLE

[Fabrízio de Royes Mello]

On Mon, Feb 29, 2016 at 2:58 PM, Fabrízio de Royes Mello <
fabriziome...@gmail.com> wrote:
>
> Hi all,
>
> Some time ago we added [1] the infrastructure to allow different lock
levels for relation options.
>
> So per discussion [2] the attached patch reduce lock levels down to
ShareUpdateExclusiveLock for:
> - fastupdate
> - fillfactor
> - gin_pending_list_limit
> - seq_page_cost
> - random_page_cost
> - n_distinct
> - n_distinct_inherited
> - buffering
>
> Att,
>
>
> [1]
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=47167b7907a802ed39b179c8780b76359468f076
> [2]
http://www.postgresql.org/message-id/20150731022857.gc11...@alap3.anarazel.de
>

Added to the next commitfest:

https://commitfest.postgresql.org/9/558/

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

[HACKERS] Reduce lock levels others reloptions in ALTER TABLE

[Fabrízio de Royes Mello]

Hi all,

Some time ago we added [1] the infrastructure to allow different lock
levels for relation options.

So per discussion [2] the attached patch reduce lock levels down to
ShareUpdateExclusiveLock for:
- fastupdate
- fillfactor
- gin_pending_list_limit
- seq_page_cost
- random_page_cost
- n_distinct
- n_distinct_inherited
- buffering

Att,


[1]
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=47167b7907a802ed39b179c8780b76359468f076
[2]
http://www.postgresql.org/message-id/20150731022857.gc11...@alap3.anarazel.de


-- 
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 86b9ae1..8128dd4 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -76,7 +76,7 @@ static relopt_bool boolRelOpts[] =
 			"fastupdate",
 			"Enables \"fast update\" feature for this GIN index",
 			RELOPT_KIND_GIN,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		true
 	},
@@ -100,7 +100,7 @@ static relopt_int intRelOpts[] =
 			"fillfactor",
 			"Packs table pages only to this percentage",
 			RELOPT_KIND_HEAP,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100
 	},
@@ -109,7 +109,7 @@ static relopt_int intRelOpts[] =
 			"fillfactor",
 			"Packs btree index pages only to this percentage",
 			RELOPT_KIND_BTREE,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		BTREE_DEFAULT_FILLFACTOR, BTREE_MIN_FILLFACTOR, 100
 	},
@@ -118,7 +118,7 @@ static relopt_int intRelOpts[] =
 			"fillfactor",
 			"Packs hash index pages only to this percentage",
 			RELOPT_KIND_HASH,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		HASH_DEFAULT_FILLFACTOR, HASH_MIN_FILLFACTOR, 100
 	},
@@ -127,7 +127,7 @@ static relopt_int intRelOpts[] =
 			"fillfactor",
 			"Packs gist index pages only to this percentage",
 			RELOPT_KIND_GIST,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		GIST_DEFAULT_FILLFACTOR, GIST_MIN_FILLFACTOR, 100
 	},
@@ -136,7 +136,7 @@ static relopt_int intRelOpts[] =
 			"fillfactor",
 			"Packs spgist index pages only to this percentage",
 			RELOPT_KIND_SPGIST,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		SPGIST_DEFAULT_FILLFACTOR, SPGIST_MIN_FILLFACTOR, 100
 	},
@@ -250,7 +250,7 @@ static relopt_int intRelOpts[] =
 			"gin_pending_list_limit",
 			"Maximum size of the pending list for this GIN index, in kilobytes.",
 			RELOPT_KIND_GIN,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		-1, 64, MAX_KILOBYTES
 	},
@@ -297,7 +297,7 @@ static relopt_real realRelOpts[] =
 			"seq_page_cost",
 			"Sets the planner's estimate of the cost of a sequentially fetched disk page.",
 			RELOPT_KIND_TABLESPACE,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		-1, 0.0, DBL_MAX
 	},
@@ -306,7 +306,7 @@ static relopt_real realRelOpts[] =
 			"random_page_cost",
 			"Sets the planner's estimate of the cost of a nonsequentially fetched disk page.",
 			RELOPT_KIND_TABLESPACE,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		-1, 0.0, DBL_MAX
 	},
@@ -315,7 +315,7 @@ static relopt_real realRelOpts[] =
 			"n_distinct",
 			"Sets the planner's estimate of the number of distinct values appearing in a column (excluding child relations).",
 			RELOPT_KIND_ATTRIBUTE,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		0, -1.0, DBL_MAX
 	},
@@ -324,7 +324,7 @@ static relopt_real realRelOpts[] =
 			"n_distinct_inherited",
 			"Sets the planner's estimate of the number of distinct values appearing in a column (including child relations).",
 			RELOPT_KIND_ATTRIBUTE,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		0, -1.0, DBL_MAX
 	},
@@ -339,7 +339,7 @@ static relopt_string stringRelOpts[] =
 			"buffering",
 			"Enables buffering build for this GiST index",
 			RELOPT_KIND_GIST,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		4,
 		false,
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 7c88ddc..3232cda 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2065,19 +2065,19 @@ select * from my_locks order by 1;
 commit;
 begin; alter table alterlock set (fillfactor = 100);
 select * from my_locks order by 1;
-  relname  |max_lockmode 
+-
- alterlock | AccessExcl

Re: [HACKERS] PostgreSQL extension API? Documentation?

[Fabrízio de Royes Mello]

On Sat, Feb 27, 2016 at 10:37 AM, Álvaro Hernández Tortosa <a...@8kdata.com>
wrote:
>
>
> Hi.
>
> I have a newbie question for extension development. Extensions
provide an entry point, and are dynamically linked to PostgreSQL. But what
APIs/functions are really available for extensions to call?
>
> The most obvious API is SPI. You could also implements hooks. Of
course, functions, types, aggregates, whatever. But can an extension call
other "internal" PostgreSQL functions? Is there any restriction to what
could --or should-- call an extension? Is there any specific API, or any
documentation which states what is available to use?
>
> In other words: what is the API surface exposed by PostgreSQL to
extension developers? The assumption is that no PostgreSQL code should be
modified, just adding your own and calling existing funcitons.
>

I don't know what kind of problem you want to solve, but maybe you should
ask to yourself:

1) I need to change some current PostgreSQL behavior?

2) I need to add a new feature do PostgreSQL without change the current
behavior?

Writing a C extension you can access a lot of internal code if it's
available internally by .h headers. For example, some time ago I'm thinking
to write an extension to show more internal information about autovacuum
(internal queue, etc... some like pg_stat_autovaccuum) . But nowadays is
impossible without change the core because some internal structures are not
exposed, so we should define an internal API to expose this kind of
information.

So depending what problem you want to solve you can write an extension to
do that. Then unfortunately the short aswer is "depend".

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] blocking the alter role command

[Fabrízio de Royes Mello]

On Tue, Jan 12, 2016 at 8:32 PM, JotaComm <jota.c...@gmail.com> wrote:
>
> Hello,
>
> I would like to know if is it possible to block the following command:
ALTER USER myuser PASSWORD;
>
> My target is allow to execute this command from a specific address.
>
> Thanks a lot.
>

You should implement an extension using ProcessUtility_hook todo that. See
an example in [1]

Regards,

[1] https://github.com/michaelpq/pg_plugins/tree/master/hook_utility

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Patch: fix lock contention for HASHHDR.mutex

[Fabrízio de Royes Mello]

On Wed, Dec 30, 2015 at 1:10 PM, Oleg Bartunov <obartu...@gmail.com> wrote:
>
>
>
> On Wed, Dec 30, 2015 at 5:44 PM, Andres Freund <and...@anarazel.de> wrote:
>>
>> On 2015-12-30 11:37:19 -0300, Alvaro Herrera wrote:
>> > Aleksander Alekseev wrote:
>> >
>> > > Here is a funny thing - benchmark results I shared 22.12.2015 are
wrong
>> > > because I forgot to run `make clean` after changing lwlock.h
(autotools
>> > > doesn't rebuild project properly after changing .h files).
>> >
>> > Running configure with --enable-depend should avoid this problem.
>>
>> I still maintain that --enable-depend should be on by default. We're
>
>
> +1
>

+1, I always use it.

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Git cartoon

[Fabrízio de Royes Mello]

Em segunda-feira, 9 de novembro de 2015, Albe Laurenz <
laurenz.a...@wien.gv.at> escreveu:

> Fabrízio de Royes Mello wrote:
> > Em domingo, 8 de novembro de 2015, Bruce Momjian <br...@momjian.us
> <javascript:;>> escreveu:
> >> This git cartoon was too funny not to share:
> >>
> >> http://xkcd.com/1597/
> >>
> >> Maybe we need it on our git wiki page.  ;-)
> >
> >  I think we need our own cartoon with a funny DB story.
>
> What, like this?
>
> http://xkcd.com/327/
>

Hahahahah... Very funny...


-- 
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Git cartoon

[Fabrízio de Royes Mello]

Em domingo, 8 de novembro de 2015, Bruce Momjian <br...@momjian.us>
escreveu:

> This git cartoon was too funny not to share:
>
> http://xkcd.com/1597/
>
> Maybe we need it on our git wiki page.  ;-)


>
 I think we need our own cartoon with a funny DB story.


-- 
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Getting sorted data from foreign server

[Fabrízio de Royes Mello]

On Tue, Oct 27, 2015 at 5:26 AM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:
>
>
>
> On Fri, Oct 23, 2015 at 2:43 AM, Robert Haas <robertmh...@gmail.com>
wrote:
>>
>> On Wed, Oct 21, 2015 at 5:23 AM, Ashutosh Bapat
>> <ashutosh.ba...@enterprisedb.com> wrote:
>> > Increasing the sorting cost factor (when use_remote_estimates = false)
from
>> > 1.1 to 1.2 makes the difference disappear.
>> >
>> > Since the startup costs for postgres_fdw are large portion of total
cost,
>> > extra 10% of rest of the cost is comparable to 1% fuzzy limit. IMO, we
>> > shouldn't bother too much about it as the path costs are not much
different.
>>
>> My feeling is that cranking the sorting cost factor up to 20-25% would
>> be a good idea, just so we have less unnecessary plan churn.  I dunno
>> if sorting always costs that much, but if a 10% cost overhead is
>> really 1% because it only applies to a fraction of the cost, I don't
>> think that's good.  The whole point was to pick something large enough
>> that we wouldn't take the sorted path unless we will benefit from the
>> sort, and clearly that's not what happened here.
>>
>
> PFA patch with the default multiplication factor for sort bumped up to
1.2.
>

+/* If no remote estimates, assume a sort costs 10% extra */
+#define DEFAULT_FDW_SORT_MULTIPLIER 1.2

The above comment should not be 20%?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] proposal: DROP DATABASE variant that kills active sessions

[Fabrízio de Royes Mello]

On Fri, Oct 16, 2015 at 4:12 PM, Robert Haas <robertmh...@gmail.com> wrote:
>
> On Fri, Oct 16, 2015 at 6:22 AM, Pavel Stehule <pavel.steh...@gmail.com>
wrote:
> > in GoodData we have this feature implemented - little bit different
named -
> > DROP DATABASE FORCE
> >
> > It is useful in complex environment with mix of pooled and not pooled
> > connections - and in our environment - about 2K databases per server
with
> > lot of dropped / created databases per server / per day.
> >
> > I can publish this patch, if there will be any interest.
>
> I think this would be a useful feature.  What would one do about
> prepared transactions?
>

Isn't "rollback all prepared" before an option?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Can pg_dump make use of CURRENT/SESSION_USER

[Fabrízio de Royes Mello]

On Thu, Sep 3, 2015 at 8:16 AM, Andres Freund <and...@anarazel.de> wrote:
>
> On 2015-08-25 22:01:51 +0900, Michael Paquier wrote:
> > Seeing no activity in the last couple of months for this patch that
> > had reviews, it is now marked as returned with feedback.
>
> Fabrizio, you after the above moved the patch to next commitfest,
> without a new patch or a additional discussion. Why? Just dragging
> patches through commitfest justincreases the work for a number of people
> (this round me) without a benefit, so that's really not a good idea.
>

My intention was move to next commitfest and finish the "rework", but I
didn't finish it due some other professional priorities.

Sorry by the noise.


> I'm marking the patch as returned with feedback again. Once there's a
> new patch we can deal with it in the appropriate commitfest.
>

Ok.


Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Is this a bug?

[Fabrízio de Royes Mello]

On Mon, Aug 25, 2014 at 6:07 PM, Bruce Momjian br...@momjian.us wrote:

 On Fri, Aug 22, 2014 at 10:04:50PM -0400, Bruce Momjian wrote:
  On Fri, Aug 22, 2014 at 03:12:47PM -0400, Robert Haas wrote:
   On Fri, Aug 22, 2014 at 2:33 PM, Bruce Momjian br...@momjian.us
wrote:
Yes, you remember well.  I will have to find a different way for
pg_upgrade to call a no-op ALTER TABLE, which is fine.
   
Looking at the ALTER TABLE options, I am going to put this check in
a
!IsBinaryUpgrade block so pg_upgrade can still use its trick.
  
   -1, that's really ugly.
  
   Maybe the right solution is to add a form of ALTER TABLE that is
   specifically defined to do only this check.  This is an ongoing need,
   so that might not be out of line.
 
  Ah, seems ALTER TABLE ... DROP CONSTRAINT IF EXISTS also works --- I
  will use that.

 OK, attached patch applied, with pg_upgrade adjustments.  I didn't
 think the original regression tests for this were necessary.


Hi,

Why this patch was reverted one day after applied [1]? I didn't see any
discussion around it.

Regards,

[1]
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=6cb74a67e26523eb2408f441bfc589c80f76c465

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Timbira: http://www.timbira.com.br
 Blog: http://fabriziomello.github.io
 Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello
 Github: http://github.com/fabriziomello

Re: [HACKERS] Is this a bug?

[Fabrízio de Royes Mello]

On Wed, Aug 26, 2015 at 4:30 PM, Andres Freund and...@anarazel.de wrote:

 On 2015-08-26 16:24:31 -0300, Fabrízio de Royes Mello wrote:
  Why this patch was reverted one day after applied [1]? I didn't see any
  discussion around it.

 http://www.postgresql.org/message-id/23918.1409010...@sss.pgh.pa.us


Thanks I'm not subscribed to pgsql-commiters so I didn't see it.

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Timbira: http://www.timbira.com.br
 Blog: http://fabriziomello.github.io
 Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello
 Github: http://github.com/fabriziomello

Re: [HACKERS] Is this a bug?

[Fabrízio de Royes Mello]

On Wed, Aug 26, 2015 at 4:31 PM, Alvaro Herrera alvhe...@2ndquadrant.com
wrote:

 Fabrízio de Royes Mello wrote:

  Why this patch was reverted one day after applied [1]? I didn't see any
  discussion around it.

 Contributors whose patches are getting committed should really subscribe
 to pgsql-committers.


Ok. I sent a subscribe to pgsql-committers.

Thanks,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Timbira: http://www.timbira.com.br
 Blog: http://fabriziomello.github.io
 Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello
 Github: http://github.com/fabriziomello

Re: [HACKERS] Commitfest remaining Needs Review items

[Fabrízio de Royes Mello]

On Tue, Aug 25, 2015 at 4:39 AM, Michael Paquier michael.paqu...@gmail.com
wrote:

 [...]

 -- Support to COMMENT ON CURRENT DATABASE: returned with feedback? The
 author mentioned that patch will be reworked but there has been no new
 version around it seems.


Moved to the next commitfest.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Timbira: http://www.timbira.com.br
 Blog: http://fabriziomello.github.io
 Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello
 Github: http://github.com/fabriziomello

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

On Wed, Aug 5, 2015 at 9:21 PM, Michael Paquier michael.paqu...@gmail.com
wrote:

 On Thu, Aug 6, 2015 at 3:06 AM, Fabrízio de Royes Mello wrote:
  On Wed, Aug 5, 2015 at 9:31 AM, Robert Haas wrote:
  Agreed.  I think we're making a mountain out of a molehill here.  As
  long as the locks that are actually used are monotonic, just use  and
  stick a comment in there explaining that it could need adjustment if
  we use other lock levels in the future.  I presume all the lock-levels
  used for DDL are, and will always be, self-exclusive, so why all this
  hand-wringing?
 
 
  New version attached with suggested changes.

 Thanks!

 +# SET autovacuum_* options needs a ShareUpdateExclusiveLock
 +# so we mix reads with it to see what works or waits
 s/needs/need/ and I think you mean mixing writes, not reads.

 Those are minor things though, and from my point of view a committer
 can look at it.


Fixed. Thanks for your review.

Regards,

*** This work is funded by Zenvia Mobile Results (http://www.zenvia.com.br)

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Timbira: http://www.timbira.com.br
 Blog: http://fabriziomello.github.io
 Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello
 Github: http://github.com/fabriziomello
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1c1c181..ad985cd 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -543,6 +543,10 @@ ALTER TABLE ALL IN TABLESPACE replaceable class=PARAMETERname/replaceable
   of commandALTER TABLE/ that forces a table rewrite.
  /para
 
+ para
+  Changing autovacuum storage parameters acquires a literalSHARE UPDATE EXCLUSIVE/literal lock.
+ /para
+
  note
   para
While commandCREATE TABLE/ allows literalOIDS/ to be specified
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 180f529..c39b878 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -57,7 +57,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			autovacuum_enabled,
 			Enables autovacuum in this relation,
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			ShareUpdateExclusiveLock
 		},
 		true
 	},
@@ -65,7 +66,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			user_catalog_table,
 			Declare a table as an additional catalog table, e.g. for the purpose of logical replication,
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -73,7 +75,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			fastupdate,
 			Enables \fast update\ feature for this GIN index,
-			RELOPT_KIND_GIN
+			RELOPT_KIND_GIN,
+			AccessExclusiveLock
 		},
 		true
 	},
@@ -81,7 +84,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			security_barrier,
 			View acts as a row security barrier,
-			RELOPT_KIND_VIEW
+			RELOPT_KIND_VIEW,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -95,7 +99,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs table pages only to this percentage,
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100
 	},
@@ -103,7 +108,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs btree index pages only to this percentage,
-			RELOPT_KIND_BTREE
+			RELOPT_KIND_BTREE,
+			AccessExclusiveLock
 		},
 		BTREE_DEFAULT_FILLFACTOR, BTREE_MIN_FILLFACTOR, 100
 	},
@@ -111,7 +117,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs hash index pages only to this percentage,
-			RELOPT_KIND_HASH
+			RELOPT_KIND_HASH,
+			AccessExclusiveLock
 		},
 		HASH_DEFAULT_FILLFACTOR, HASH_MIN_FILLFACTOR, 100
 	},
@@ -119,7 +126,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs gist index pages only to this percentage,
-			RELOPT_KIND_GIST
+			RELOPT_KIND_GIST,
+			AccessExclusiveLock
 		},
 		GIST_DEFAULT_FILLFACTOR, GIST_MIN_FILLFACTOR, 100
 	},
@@ -127,7 +135,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs spgist index pages only to this percentage,
-			RELOPT_KIND_SPGIST
+			RELOPT_KIND_SPGIST,
+			AccessExclusiveLock
 		},
 		SPGIST_DEFAULT_FILLFACTOR, SPGIST_MIN_FILLFACTOR, 100
 	},
@@ -135,7 +144,8 @@ static relopt_int intRelOpts[] =
 		{
 			autovacuum_vacuum_threshold,
 			Minimum number of tuple updates or deletes prior to vacuum,
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			ShareUpdateExclusiveLock
 		},
 		-1, 0, INT_MAX
 	},
@@ -143,7 +153,8 @@ static relopt_int intRelOpts[] =
 		{
 			autovacuum_analyze_threshold,
 			Minimum number of tuple inserts, updates or deletes prior to analyze,
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			ShareUpdateExclusiveLock
 		},
 		-1, 0, INT_MAX
 	},
@@ -151,7 +162,8 @@ static relopt_int intRelOpts[] =
 		{
 			autovacuum_vacuum_cost_delay,
 			Vacuum cost delay

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

On Wed, Aug 5, 2015 at 9:31 AM, Robert Haas robertmh...@gmail.com wrote:

 On Tue, Aug 4, 2015 at 1:15 PM, Alvaro Herrera alvhe...@2ndquadrant.com
wrote:
  That opens up for lock escalation and deadlocks, doesn't it?  You are
  probably thinking that it's okay to ignore those but I don't necessarily
  agree with that.

 Agreed.  I think we're making a mountain out of a molehill here.  As
 long as the locks that are actually used are monotonic, just use  and
 stick a comment in there explaining that it could need adjustment if
 we use other lock levels in the future.  I presume all the lock-levels
 used for DDL are, and will always be, self-exclusive, so why all this
 hand-wringing?


New version attached with suggested changes.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Timbira: http://www.timbira.com.br
 Blog: http://fabriziomello.github.io
 Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello
 Github: http://github.com/fabriziomello
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1c1c181..ad985cd 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -543,6 +543,10 @@ ALTER TABLE ALL IN TABLESPACE replaceable class=PARAMETERname/replaceable
   of commandALTER TABLE/ that forces a table rewrite.
  /para
 
+ para
+  Changing autovacuum storage parameters acquires a literalSHARE UPDATE EXCLUSIVE/literal lock.
+ /para
+
  note
   para
While commandCREATE TABLE/ allows literalOIDS/ to be specified
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 180f529..c39b878 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -57,7 +57,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			autovacuum_enabled,
 			Enables autovacuum in this relation,
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			ShareUpdateExclusiveLock
 		},
 		true
 	},
@@ -65,7 +66,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			user_catalog_table,
 			Declare a table as an additional catalog table, e.g. for the purpose of logical replication,
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -73,7 +75,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			fastupdate,
 			Enables \fast update\ feature for this GIN index,
-			RELOPT_KIND_GIN
+			RELOPT_KIND_GIN,
+			AccessExclusiveLock
 		},
 		true
 	},
@@ -81,7 +84,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			security_barrier,
 			View acts as a row security barrier,
-			RELOPT_KIND_VIEW
+			RELOPT_KIND_VIEW,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -95,7 +99,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs table pages only to this percentage,
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100
 	},
@@ -103,7 +108,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs btree index pages only to this percentage,
-			RELOPT_KIND_BTREE
+			RELOPT_KIND_BTREE,
+			AccessExclusiveLock
 		},
 		BTREE_DEFAULT_FILLFACTOR, BTREE_MIN_FILLFACTOR, 100
 	},
@@ -111,7 +117,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs hash index pages only to this percentage,
-			RELOPT_KIND_HASH
+			RELOPT_KIND_HASH,
+			AccessExclusiveLock
 		},
 		HASH_DEFAULT_FILLFACTOR, HASH_MIN_FILLFACTOR, 100
 	},
@@ -119,7 +126,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs gist index pages only to this percentage,
-			RELOPT_KIND_GIST
+			RELOPT_KIND_GIST,
+			AccessExclusiveLock
 		},
 		GIST_DEFAULT_FILLFACTOR, GIST_MIN_FILLFACTOR, 100
 	},
@@ -127,7 +135,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs spgist index pages only to this percentage,
-			RELOPT_KIND_SPGIST
+			RELOPT_KIND_SPGIST,
+			AccessExclusiveLock
 		},
 		SPGIST_DEFAULT_FILLFACTOR, SPGIST_MIN_FILLFACTOR, 100
 	},
@@ -135,7 +144,8 @@ static relopt_int intRelOpts[] =
 		{
 			autovacuum_vacuum_threshold,
 			Minimum number of tuple updates or deletes prior to vacuum,
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			ShareUpdateExclusiveLock
 		},
 		-1, 0, INT_MAX
 	},
@@ -143,7 +153,8 @@ static relopt_int intRelOpts[] =
 		{
 			autovacuum_analyze_threshold,
 			Minimum number of tuple inserts, updates or deletes prior to analyze,
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			ShareUpdateExclusiveLock
 		},
 		-1, 0, INT_MAX
 	},
@@ -151,7 +162,8 @@ static relopt_int intRelOpts[] =
 		{
 			autovacuum_vacuum_cost_delay,
 			Vacuum cost delay in milliseconds, for autovacuum,
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			ShareUpdateExclusiveLock
 		},
 		-1, 0, 100
 	},
@@ -159,7 +171,8 @@ static relopt_int intRelOpts[] =
 		{
 			autovacuum_vacuum_cost_limit,
 			Vacuum cost

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

On Tue, Aug 4, 2015 at 5:55 AM, Andres Freund and...@anarazel.de wrote:

 On 2015-08-03 14:15:27 +0900, Michael Paquier wrote:
  On Sat, Aug 1, 2015 at 9:20 PM, Andres Freund wrote:
   On August 1, 2015 2:17:24 PM GMT+02:00, Michael Paquier wrote:
   For instance, if you told me to choose between ShareLock and
   ShareUpdateExclusiveLock I wouldn't know which one is strongest.  I
   don't it's sensible to have the lock mode compare primitive
  honestly.
   I don't have any great ideas to offer ATM sadly.
  
  Yes, the thing is that lowering the lock levels is good for
  concurrency, but the non-monotony of the lock levels makes it
  impossible to choose an intermediate state correctly.
  
   How about simply acquiring all the locks individually of they're
different types? These few acquisitions won't matter.
 
  As long as this only applies on master, this may be fine... We could
  basically pass a LOCKMASK to the multiple layers of tablecmds.c
  instead of LOCKMODE to track all the locks that need to be taken, and
  all the relations open during operations.

 This sounds far too complicated to me. Just LockRelationOid() the
 relation with the appropriate level everytime you pass through the
 function?

Hi all,

IMHO is more simply we just fallback to AccessExclusiveLock if there are
different lockmodes in reloptions as Michael suggested before.

Look at the new version attached.

Regards,


*** This work is funded by Zenvia Mobile Results (http://www.zenvia.com.br)

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Timbira: http://www.timbira.com.br
 Blog: http://fabriziomello.github.io
 Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello
 Github: http://github.com/fabriziomello
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1c1c181..ad985cd 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -543,6 +543,10 @@ ALTER TABLE ALL IN TABLESPACE replaceable class=PARAMETERname/replaceable
   of commandALTER TABLE/ that forces a table rewrite.
  /para
 
+ para
+  Changing autovacuum storage parameters acquires a literalSHARE UPDATE EXCLUSIVE/literal lock.
+ /para
+
  note
   para
While commandCREATE TABLE/ allows literalOIDS/ to be specified
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 180f529..e0f9f09 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -57,7 +57,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			autovacuum_enabled,
 			Enables autovacuum in this relation,
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			ShareUpdateExclusiveLock
 		},
 		true
 	},
@@ -65,7 +66,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			user_catalog_table,
 			Declare a table as an additional catalog table, e.g. for the purpose of logical replication,
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -73,7 +75,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			fastupdate,
 			Enables \fast update\ feature for this GIN index,
-			RELOPT_KIND_GIN
+			RELOPT_KIND_GIN,
+			AccessExclusiveLock
 		},
 		true
 	},
@@ -81,7 +84,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			security_barrier,
 			View acts as a row security barrier,
-			RELOPT_KIND_VIEW
+			RELOPT_KIND_VIEW,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -95,7 +99,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs table pages only to this percentage,
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100
 	},
@@ -103,7 +108,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs btree index pages only to this percentage,
-			RELOPT_KIND_BTREE
+			RELOPT_KIND_BTREE,
+			AccessExclusiveLock
 		},
 		BTREE_DEFAULT_FILLFACTOR, BTREE_MIN_FILLFACTOR, 100
 	},
@@ -111,7 +117,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs hash index pages only to this percentage,
-			RELOPT_KIND_HASH
+			RELOPT_KIND_HASH,
+			AccessExclusiveLock
 		},
 		HASH_DEFAULT_FILLFACTOR, HASH_MIN_FILLFACTOR, 100
 	},
@@ -119,7 +126,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs gist index pages only to this percentage,
-			RELOPT_KIND_GIST
+			RELOPT_KIND_GIST,
+			AccessExclusiveLock
 		},
 		GIST_DEFAULT_FILLFACTOR, GIST_MIN_FILLFACTOR, 100
 	},
@@ -127,7 +135,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs spgist index pages only to this percentage,
-			RELOPT_KIND_SPGIST
+			RELOPT_KIND_SPGIST,
+			AccessExclusiveLock
 		},
 		SPGIST_DEFAULT_FILLFACTOR, SPGIST_MIN_FILLFACTOR, 100
 	},
@@ -135,7 +144,8 @@ static relopt_int intRelOpts[] =
 		{
 			autovacuum_vacuum_threshold,
 			Minimum number of tuple updates or deletes prior to vacuum,
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

On Mon, Aug 3, 2015 at 2:15 AM, Michael Paquier michael.paqu...@gmail.com
wrote:

 On Sat, Aug 1, 2015 at 9:20 PM, Andres Freund wrote:
  On August 1, 2015 2:17:24 PM GMT+02:00, Michael Paquier wrote:
  For instance, if you told me to choose between ShareLock and
  ShareUpdateExclusiveLock I wouldn't know which one is strongest.  I
  don't it's sensible to have the lock mode compare primitive
 honestly.
  I don't have any great ideas to offer ATM sadly.
 
 Yes, the thing is that lowering the lock levels is good for
 concurrency, but the non-monotony of the lock levels makes it
 impossible to choose an intermediate state correctly.
 
  How about simply acquiring all the locks individually of they're
different types? These few acquisitions won't matter.

 As long as this only applies on master, this may be fine... We could
 basically pass a LOCKMASK to the multiple layers of tablecmds.c
 instead of LOCKMODE to track all the locks that need to be taken, and
 all the relations open during operations. Would it be worth having
 some routines like relation_multi_[open|close]() as well? Like that:
 Relation[] relation_multi_open(relation Oid, LOCKMASK):
 void relation_multi_close(Relation[]);

 If we do something, we may consider patching as well 9.5, it seems to
 me that tablecmds.c is broken by assuming that lock hierarchy is
 monotone in AlterTableGetLockLevel().


Hey guys,

Are you sure we need to do all this changes just to check the highest
locklevel based on the reloptions? Or I misunderstood the whole thing?

Maybe we just check the DoLockModeConflict in the new method
GetReloptionsLockLevel and if true then fallback to AccessExclusiveLock (as
Michael suggested in a previous email).


Regards,


*** This work is funded by Zenvia Mobile Results (http://www.zenvia.com.br)

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Timbira: http://www.timbira.com.br
 Blog: http://fabriziomello.github.io
 Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello
 Github: http://github.com/fabriziomello

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

On Fri, Jul 31, 2015 at 10:01 AM, Michael Paquier michael.paqu...@gmail.com
wrote:

 On Fri, Jul 31, 2015 at 11:41 AM, Fabrízio de Royes Mello wrote:
  We usually don't compare lock values that way, i.e. there's not
  guaranteed to be a strict monotonicity between lock levels. I don't
  really agree with that policy, but it's nonetheless there.
 
  And how is the better way to compare lock values to get the highest lock
  level? Perhaps creating a function to compare lock levels?

 I guess that this is exactly what Andres has in mind, aka something
 like LockModeCompare(lockmode, lockmode) that returns {-1,0,1}
 depending on which lock is higher on the hierarchy. This would do
 exactly what your patch is doing though, except that this will
 localize the comparison operators in lock.c. Though I am seeing at
 quick glance a couple of places already do such comparisons:
 backend/commands/tablecmds.c:if (cmd_lockmode  lockmode)
 backend/storage/lmgr/lock.c:lockmode  RowExclusiveLock)
 backend/storage/lmgr/lock.c:if (lockmode = AccessExclusiveLock 
 backend/access/heap/heapam.c:Assert(lockmode = NoLock  lockmode
  MAX_LOCKMODES);
 backend/access/heap/heapam.c:Assert(lockmode = NoLock  lockmode
  MAX_LOCKMODES);
 backend/access/heap/heapam.c:Assert(lockmode = NoLock  lockmode
  MAX_LOCKMODES);
 backend/access/index/indexam.c:Assert(lockmode = NoLock 
 lockmode  MAX_LOCKMODES);
 All of them are just sanity checks, except the one in tablecmds.c is
 not (2dbbda0). Hence I am thinking that this is not really a problem
 this patch should tackle by itself...


I did it in the attached version of the patch... But I don't know if the
names are good so fell free to suggest others if you dislike of my choice.

In this patch I didn't change all lockmode comparison places previous
pointed by you, but I can change it maybe adding other method called
LockModeIsValid(lockmode) to do the comparison lockmode = NoLock 
lockmode  MAX_LOCKMODES used in many places.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Timbira: http://www.timbira.com.br
 Blog: http://fabriziomello.github.io
 Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello
 Github: http://github.com/fabriziomello
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1c1c181..ad985cd 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -543,6 +543,10 @@ ALTER TABLE ALL IN TABLESPACE replaceable class=PARAMETERname/replaceable
   of commandALTER TABLE/ that forces a table rewrite.
  /para
 
+ para
+  Changing autovacuum storage parameters acquires a literalSHARE UPDATE EXCLUSIVE/literal lock.
+ /para
+
  note
   para
While commandCREATE TABLE/ allows literalOIDS/ to be specified
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 8176b6a..a62ada1 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -57,7 +57,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			autovacuum_enabled,
 			Enables autovacuum in this relation,
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			ShareUpdateExclusiveLock
 		},
 		true
 	},
@@ -65,7 +66,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			user_catalog_table,
 			Declare a table as an additional catalog table, e.g. for the purpose of logical replication,
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -73,7 +75,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			fastupdate,
 			Enables \fast update\ feature for this GIN index,
-			RELOPT_KIND_GIN
+			RELOPT_KIND_GIN,
+			AccessExclusiveLock
 		},
 		true
 	},
@@ -81,7 +84,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			security_barrier,
 			View acts as a row security barrier,
-			RELOPT_KIND_VIEW
+			RELOPT_KIND_VIEW,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -95,7 +99,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs table pages only to this percentage,
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100
 	},
@@ -103,7 +108,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs btree index pages only to this percentage,
-			RELOPT_KIND_BTREE
+			RELOPT_KIND_BTREE,
+			AccessExclusiveLock
 		},
 		BTREE_DEFAULT_FILLFACTOR, BTREE_MIN_FILLFACTOR, 100
 	},
@@ -111,7 +117,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs hash index pages only to this percentage,
-			RELOPT_KIND_HASH
+			RELOPT_KIND_HASH,
+			AccessExclusiveLock
 		},
 		HASH_DEFAULT_FILLFACTOR, HASH_MIN_FILLFACTOR, 100
 	},
@@ -119,7 +126,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs gist index pages only to this percentage,
-			RELOPT_KIND_GIST
+			RELOPT_KIND_GIST,
+			AccessExclusiveLock

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

)
  +{
  + LOCKMODElockmode = NoLock;
  + ListCell*cell;
  +
  + if (defList == NIL)
  + return AccessExclusiveLock;
  +
  + if (need_initialization)
  + initialize_reloptions();
  +
  + foreach(cell, defList)
  + {
  + DefElem *def = (DefElem *) lfirst(cell);
  + int i;
  +
  + for (i = 0; relOpts[i]; i++)
  + {
  + if (pg_strncasecmp(relOpts[i]-name,
def-defname, relOpts[i]-namelen + 1) == 0)
  + {
  + if (lockmode  relOpts[i]-lockmode)
  + lockmode = relOpts[i]-lockmode;
  + }
  + }
  + }
  +
  + return lockmode;
  +}

 We usually don't compare lock values that way, i.e. there's not
 guaranteed to be a strict monotonicity between lock levels. I don't
 really agree with that policy, but it's nonetheless there.


And how is the better way to compare lock values to get the highest lock
level? Perhaps creating a function to compare lock levels?

Regards,

*** This work is funded by Zenvia Mobile Results (http://www.zenvia.com.br)

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Timbira: http://www.timbira.com.br
 Blog: http://fabriziomello.github.io
 Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello
 Github: http://github.com/fabriziomello

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

On Thu, Jul 30, 2015 at 10:46 PM, Michael Paquier michael.paqu...@gmail.com
wrote:

 This patch size has increased from 16k to 157k because of the output
 of the isolation tests you just added. This is definitely too large
 and actually the test coverage is rather limited. Hence I think that
 they should be changed as follows:
 - Use only one table, the locks of tables a and b do not conflict, and
 that's what we want to look at here.
 - Check the locks of all the relation parameters, by that I mean as
 well fillfactor and user_catalog_table which still take
 AccessExclusiveLock on the relation
 - Use custom permutations. I doubt that there is much sense to test
 all the permutations in this context, and this will reduce the
 expected output size drastically.


Ok.


 On further notice, I would recommend not to use the same string name
 for the session and the query identifiers. And I think that inserting
 only one tuple at initialization is just but fine.

 Here is an example of such a spec:
 setup
 {
  CREATE TABLE a (id int PRIMARY KEY);
  INSERT INTO a SELECT generate_series(1,100);
 }
 teardown
 {
  DROP TABLE a;
 }
 session s1
 step b1   { BEGIN; }
 # TODO add one query per parameter
 step at11 { ALTER TABLE a SET (fillfactor=10); }
 step at12 { ALTER TABLE a SET
(autovacuum_vacuum_scale_factor=0.001); }
 step c1   { COMMIT; }
 session s2
 step b2   { BEGIN; }
 step wx1  { UPDATE a SET id = id + 1; }
 step c2   { COMMIT; }

 And by testing permutations like for example that it is possible to
 see which session is waiting for what. Input:
 permutation b1 b2 at11 wx1 c1 c2
 Output where session 2 waits for lock taken after fillfactor update:
 step b1: BEGIN;
 step b2: BEGIN;
 step at11: ALTER TABLE a SET (fillfactor=10);
 step wx1: UPDATE a SET id = id + 1; waiting ...
 step c1: COMMIT;
 step wx1: ... completed
 step c2: COMMIT;

 Be careful as well to not include incorrect permutations in the
 expected output file, the isolation tester is smart enough to ping you
 about that.


Changed the isolation tests according your suggestions.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Timbira: http://www.timbira.com.br
 Blog: http://fabriziomello.github.io
 Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello
 Github: http://github.com/fabriziomello
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1c1c181..ad985cd 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -543,6 +543,10 @@ ALTER TABLE ALL IN TABLESPACE replaceable class=PARAMETERname/replaceable
   of commandALTER TABLE/ that forces a table rewrite.
  /para
 
+ para
+  Changing autovacuum storage parameters acquires a literalSHARE UPDATE EXCLUSIVE/literal lock.
+ /para
+
  note
   para
While commandCREATE TABLE/ allows literalOIDS/ to be specified
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 8176b6a..b8d2a92 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -57,7 +57,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			autovacuum_enabled,
 			Enables autovacuum in this relation,
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			ShareUpdateExclusiveLock
 		},
 		true
 	},
@@ -65,7 +66,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			user_catalog_table,
 			Declare a table as an additional catalog table, e.g. for the purpose of logical replication,
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -73,7 +75,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			fastupdate,
 			Enables \fast update\ feature for this GIN index,
-			RELOPT_KIND_GIN
+			RELOPT_KIND_GIN,
+			AccessExclusiveLock
 		},
 		true
 	},
@@ -81,7 +84,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			security_barrier,
 			View acts as a row security barrier,
-			RELOPT_KIND_VIEW
+			RELOPT_KIND_VIEW,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -95,7 +99,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs table pages only to this percentage,
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100
 	},
@@ -103,7 +108,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs btree index pages only to this percentage,
-			RELOPT_KIND_BTREE
+			RELOPT_KIND_BTREE,
+			AccessExclusiveLock
 		},
 		BTREE_DEFAULT_FILLFACTOR, BTREE_MIN_FILLFACTOR, 100
 	},
@@ -111,7 +117,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs hash index pages only to this percentage,
-			RELOPT_KIND_HASH
+			RELOPT_KIND_HASH,
+			AccessExclusiveLock
 		},
 		HASH_DEFAULT_FILLFACTOR, HASH_MIN_FILLFACTOR, 100
 	},
@@ -119,7 +126,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs gist index pages only

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

On Thu, Jul 2, 2015 at 2:03 PM, Simon Riggs si...@2ndquadrant.com wrote:

 The following review has been posted through the commitfest application:
 make installcheck-world:  not tested
 Implements feature:   not tested
 Spec compliant:   not tested
 Documentation:not tested

 Looks functionally complete

 Need a test to show that ALTER TABLE works on views, as discussed on this
thread. And confirmation that pg_dump is not broken by this.

 Message-ID: 20140321205828.gb3969...@tornado.leadboat.com


Added more test cases to cover ALTER TABLE on views.

I'm thinking about the isolation tests, what about add another
'alter-table' spec for isolation tests enabling and disabling 'autovacuum'
options?

I did some tests using ALTER TABLE on views and also ALTER VIEW and I
didn't identify any anomalies.


 Needs documentation


Added.

Att,


*** This work is funded by Zenvia Mobile Results (http://www.zenvia.com.br)

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Timbira: http://www.timbira.com.br
 Blog: http://fabriziomello.github.io
 Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello
 Github: http://github.com/fabriziomello
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 207fec1..b126855 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -541,6 +541,10 @@ ALTER TABLE ALL IN TABLESPACE replaceable class=PARAMETERname/replaceable
   of commandALTER TABLE/ that forces a table rewrite.
  /para
 
+ para
+  Changing autovacuum storage parameters acquires a literalSHARE UPDATE EXCLUSIVE/literal lock.
+ /para
+
  note
   para
While commandCREATE TABLE/ allows literalOIDS/ to be specified
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 8176b6a..f273944 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -57,7 +57,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			autovacuum_enabled,
 			Enables autovacuum in this relation,
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			ShareUpdateExclusiveLock
 		},
 		true
 	},
@@ -65,7 +66,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			user_catalog_table,
 			Declare a table as an additional catalog table, e.g. for the purpose of logical replication,
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -73,7 +75,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			fastupdate,
 			Enables \fast update\ feature for this GIN index,
-			RELOPT_KIND_GIN
+			RELOPT_KIND_GIN,
+			AccessExclusiveLock
 		},
 		true
 	},
@@ -81,7 +84,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			security_barrier,
 			View acts as a row security barrier,
-			RELOPT_KIND_VIEW
+			RELOPT_KIND_VIEW,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -95,7 +99,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs table pages only to this percentage,
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100
 	},
@@ -103,7 +108,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs btree index pages only to this percentage,
-			RELOPT_KIND_BTREE
+			RELOPT_KIND_BTREE,
+			AccessExclusiveLock
 		},
 		BTREE_DEFAULT_FILLFACTOR, BTREE_MIN_FILLFACTOR, 100
 	},
@@ -111,7 +117,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs hash index pages only to this percentage,
-			RELOPT_KIND_HASH
+			RELOPT_KIND_HASH,
+			AccessExclusiveLock
 		},
 		HASH_DEFAULT_FILLFACTOR, HASH_MIN_FILLFACTOR, 100
 	},
@@ -119,7 +126,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs gist index pages only to this percentage,
-			RELOPT_KIND_GIST
+			RELOPT_KIND_GIST,
+			AccessExclusiveLock
 		},
 		GIST_DEFAULT_FILLFACTOR, GIST_MIN_FILLFACTOR, 100
 	},
@@ -127,7 +135,8 @@ static relopt_int intRelOpts[] =
 		{
 			fillfactor,
 			Packs spgist index pages only to this percentage,
-			RELOPT_KIND_SPGIST
+			RELOPT_KIND_SPGIST,
+			AccessExclusiveLock
 		},
 		SPGIST_DEFAULT_FILLFACTOR, SPGIST_MIN_FILLFACTOR, 100
 	},
@@ -135,7 +144,8 @@ static relopt_int intRelOpts[] =
 		{
 			autovacuum_vacuum_threshold,
 			Minimum number of tuple updates or deletes prior to vacuum,
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			ShareUpdateExclusiveLock
 		},
 		-1, 0, INT_MAX
 	},
@@ -143,7 +153,8 @@ static relopt_int intRelOpts[] =
 		{
 			autovacuum_analyze_threshold,
 			Minimum number of tuple inserts, updates or deletes prior to analyze,
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			ShareUpdateExclusiveLock
 		},
 		-1, 0, INT_MAX
 	},
@@ -151,7 +162,8 @@ static relopt_int intRelOpts[] =
 		{
 			autovacuum_vacuum_cost_delay,
 			Vacuum cost delay in milliseconds, for autovacuum,
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST

Re: [HACKERS] security labels on databases are bad for dump restore

[Fabrízio de Royes Mello]

On Wed, Jul 22, 2015 at 4:42 PM, Adam Brightwell 
adam.brightw...@crunchydatasolutions.com wrote:

 [...]

 Also, I think this would potentially conflict with what Fabrízio is
 doing with CURRENT_DATABASE on COMMENT, though, I think it might be a
 preferable solution.

 https://commitfest.postgresql.org/5/229/


Unfortunately this code is a bit weird and will be better to move to the
next commitfest (I have no time to improve it yet), so we can join efforts
and implement all ideas and make the reviewers life easier with a more
consistency patch.

Seems reasonable?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Timbira: http://www.timbira.com.br
 Blog: http://fabriziomello.github.io
 Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello
 Github: http://github.com/fabriziomello

Re: [HACKERS] Add CINE for ALTER TABLE ... ADD COLUMN

[Fabrízio de Royes Mello]

On Thu, Jul 16, 2015 at 10:36 PM, Michael Paquier michael.paqu...@gmail.com
wrote:

 I had a look at this patch, and here are some minor comments:
 1) In alter_table.sgml, you need a space here:
 [ IF NOT EXISTS ]replaceable

Fixed.


 2)
 +   check_for_column_name_collision(targetrelation, newattname,
false);
 (void) needs to be added in front of check_for_column_name_collision
 where its return value is not checked or static code analyzers are
 surely going to complain.

Fixed.


 3) Something minor, some lines of codes exceed 80 characters, see
 declaration of check_for_column_name_collision for example...

Fixed.


 4) This comment needs more precisions?
 /* new name should not already exist */
 -   check_for_column_name_collision(rel, colDef-colname);
 +   if (!check_for_column_name_collision(rel, colDef-colname,
 if_not_exists))
 The new name can actually exist if if_not_exists is true.


Improved the comment.


 Except that the implementation looks sane to me.


Thank you for the review.

Att,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Timbira: http://www.timbira.com.br
 Blog: http://fabriziomello.github.io
 Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello
 Github: http://github.com/fabriziomello
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 207fec1..5a71c3c 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -36,7 +36,7 @@ ALTER TABLE ALL IN TABLESPACE replaceable class=PARAMETERname/replaceable
 
 phrasewhere replaceable class=PARAMETERaction/replaceable is one of:/phrase
 
-ADD [ COLUMN ] replaceable class=PARAMETERcolumn_name/replaceable replaceable class=PARAMETERdata_type/replaceable [ COLLATE replaceable class=PARAMETERcollation/replaceable ] [ replaceable class=PARAMETERcolumn_constraint/replaceable [ ... ] ]
+ADD [ COLUMN ] [ IF NOT EXISTS ] replaceable class=PARAMETERcolumn_name/replaceable replaceable class=PARAMETERdata_type/replaceable [ COLLATE replaceable class=PARAMETERcollation/replaceable ] [ replaceable class=PARAMETERcolumn_constraint/replaceable [ ... ] ]
 DROP [ COLUMN ] [ IF EXISTS ] replaceable class=PARAMETERcolumn_name/replaceable [ RESTRICT | CASCADE ]
 ALTER [ COLUMN ] replaceable class=PARAMETERcolumn_name/replaceable [ SET DATA ] TYPE replaceable class=PARAMETERdata_type/replaceable [ COLLATE replaceable class=PARAMETERcollation/replaceable ] [ USING replaceable class=PARAMETERexpression/replaceable ]
 ALTER [ COLUMN ] replaceable class=PARAMETERcolumn_name/replaceable SET DEFAULT replaceable class=PARAMETERexpression/replaceable
@@ -96,11 +96,12 @@ ALTER TABLE ALL IN TABLESPACE replaceable class=PARAMETERname/replaceable
 
   variablelist
varlistentry
-termliteralADD COLUMN/literal/term
+termliteralADD COLUMN [ IF NOT EXISTS ]/literal/term
 listitem
  para
   This form adds a new column to the table, using the same syntax as
-  xref linkend=SQL-CREATETABLE.
+  xref linkend=SQL-CREATETABLE. If literalIF NOT EXISTS/literal
+  is specified and the column already exists, no error is thrown.
  /para
 /listitem
/varlistentry
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1c7eded..05fbe51 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -328,8 +328,9 @@ static void ATPrepAddColumn(List **wqueue, Relation rel, bool recurse, bool recu
 bool is_view, AlterTableCmd *cmd, LOCKMODE lockmode);
 static ObjectAddress ATExecAddColumn(List **wqueue, AlteredTableInfo *tab,
 Relation rel, ColumnDef *colDef, bool isOid,
-bool recurse, bool recursing, LOCKMODE lockmode);
-static void check_for_column_name_collision(Relation rel, const char *colname);
+bool recurse, bool recursing, bool if_not_exists, LOCKMODE lockmode);
+static bool check_for_column_name_collision(Relation rel, const char *colname,
+bool if_not_exists);
 static void add_column_datatype_dependency(Oid relid, int32 attnum, Oid typid);
 static void add_column_collation_dependency(Oid relid, int32 attnum, Oid collid);
 static void ATPrepAddOids(List **wqueue, Relation rel, bool recurse,
@@ -2304,7 +2305,7 @@ renameatt_internal(Oid myrelid,
 		oldattname)));
 
 	/* new name should not already exist */
-	check_for_column_name_collision(targetrelation, newattname);
+	(void) check_for_column_name_collision(targetrelation, newattname, false);
 
 	/* apply the update */
 	namestrcpy((attform-attname), newattname);
@@ -3455,11 +3456,11 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
 		case AT_AddColumnToView:		/* add column via CREATE OR REPLACE
 		 * VIEW */
 			address = ATExecAddColumn(wqueue, tab, rel, (ColumnDef *) cmd-def,
-	  false, false, false, lockmode);
+	  false, false, false, false, lockmode);
 			break;
 		case AT_AddColumnRecurse:
 			address