forcing a rebuild of the visibility map

On Thu, Jun 16, 2016 at 12:06 PM, Robert Haas <robertmhaas@gmail.com> wrote:

[ Changing subject line in the hopes of keeping separate issues in
separate threads. ]

On Mon, Jun 6, 2016 at 11:00 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

I'm intuitively sympathetic to the idea that we should have an option
for this, but I can't figure out in what case we'd actually tell
anyone to use it. It would be useful for the kinds of bugs listed
above to have VACUUM (rebuild_vm) to blow away the VM fork and rebuild
it, but that's different semantics than what we proposed for VACUUM
(even_frozen_pages). And I'd be sort of inclined to handle that case
by providing some other way to remove VM forks (like a new function in
the pg_visibilitymap contrib module, maybe?) and then just tell people
to run regular VACUUM afterwards, rather than putting the actual VM
fork removal into VACUUM.

There's a lot to be said for that approach. If we do it, I'd be a bit
inclined to offer an option to blow away the FSM as well.

After having been scared out of my mind by the discovery of
longstanding breakage in heap_update[1], it occurred to me that this
is an excellent example of a case in which the option for which Andres
was agitating - specifically forcing VACUUM to visit ever single page
in the heap - would be useful. Even if there's functionality
available to blow away the visibility map forks, you might not want to
just go remove them all and re-vacuum everything, because while you
were rebuilding the VM forks, index-only scans would stop being
index-only, and that might cause an operational problem. Being able
to simply force every page to be visited is better. Patch for that
attached. I decided to call the option disable_page_skipping rather
than even_frozen_pages because it should also force visiting pages
that are all-visible but not all-frozen. (I was sorely tempted to go
with the competing proposal of calling it VACUUM (THEWHOLEDAMNTHING)
but I refrained.)

However, I also think that the approach described above - providing a
way to excise VM forks specifically - is useful. Patch for that
attached, too. It turns out that can't be done without either adding
a new WAL record type or extending an existing one; I chose to add a
"flags" argument to XLOG_SMGR_TRUNCATE, specifying the forks to be
truncated. Since this will require bumping the XLOG version, if we're
going to do it, and I think we should, it would be good to try to get
it done before beta2 rather than closer to release. However, I don't
want to commit this without some review, so please review this.
Actually, please review both patches.[2] The same core support could
be used to add an option to truncate the FSM, but I didn't write a
patch for that because I'm incredibly { busy | lazy }.

Option name DISABLE_PAGE_SKIPPING is good to me.
I'm still working on this, but here are some review comments.

---
+CREATE FUNCTION pg_truncate_visibility_map(regclass)
+RETURNS void
+AS 'MODULE_PATHNAME', 'pg_truncate_visibility_map'
+LANGUAGE C STRICT
+PARALLEL UNSAFE;  -- let's not make this any more dangerous
+

"REVOKE ALL ON FUNCTION pg_truncate_visibility_map(regclass) FROM
PUBLIC;" is missing in pg_visibility/pg_visibility--1.0--1.1.sql.

---
I think that VACUUM with VERBOSE option can show the information for
how many frozen pages we skipped like autovacuum does. Thought?
Patch attached.

Regards,

--
Masahiko Sawada

On Thu, Jun 16, 2016 at 2:33 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Option name DISABLE_PAGE_SKIPPING is good to me.
I'm still working on this, but here are some review comments.

---
+CREATE FUNCTION pg_truncate_visibility_map(regclass)
+RETURNS void
+AS 'MODULE_PATHNAME', 'pg_truncate_visibility_map'
+LANGUAGE C STRICT
+PARALLEL UNSAFE;  -- let's not make this any more dangerous
+

"REVOKE ALL ON FUNCTION pg_truncate_visibility_map(regclass) FROM
PUBLIC;" is missing in pg_visibility/pg_visibility--1.0--1.1.sql.

OK, thanks. I'll fix that.

I think that VACUUM with VERBOSE option can show the information for
how many frozen pages we skipped like autovacuum does. Thought?
Patch attached.

I'm not sure. The messages VACUUM emits are already quite long and
hard to read, and adding more lines to them might make that problem
worse. On the other hand, having more details can be helpful, too.

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

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

On Thu, Jun 16, 2016 at 10:03 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Jun 16, 2016 at 2:33 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Option name DISABLE_PAGE_SKIPPING is good to me.
I'm still working on this, but here are some review comments.

---
+CREATE FUNCTION pg_truncate_visibility_map(regclass)
+RETURNS void
+AS 'MODULE_PATHNAME', 'pg_truncate_visibility_map'
+LANGUAGE C STRICT
+PARALLEL UNSAFE;  -- let's not make this any more dangerous
+

"REVOKE ALL ON FUNCTION pg_truncate_visibility_map(regclass) FROM
PUBLIC;" is missing in pg_visibility/pg_visibility--1.0--1.1.sql.

OK, thanks. I'll fix that.

I think that VACUUM with VERBOSE option can show the information for
how many frozen pages we skipped like autovacuum does. Thought?
Patch attached.

I'm not sure. The messages VACUUM emits are already quite long and
hard to read, and adding more lines to them might make that problem
worse. On the other hand, having more details can be helpful, too.

Because these informations are emitted only when VERBOSE option is
specified, I think that it would be better to output that rather than
nothing, although it makes messages more complicated.

Regards,

--
Masahiko Sawada

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

Hi,

On 2016-06-15 23:06:37 -0400, Robert Haas wrote:

After having been scared out of my mind by the discovery of
longstanding breakage in heap_update[1], it occurred to me that this
is an excellent example of a case in which the option for which Andres
was agitating - specifically forcing VACUUM to visit ever single page
in the heap - would be useful. [...] . Being able
to simply force every page to be visited is better. Patch for that
attached. I decided to call the option disable_page_skipping rather
than even_frozen_pages because it should also force visiting pages
that are all-visible but not all-frozen.

However, I also think that the approach described above - providing a
way to excise VM forks specifically - is useful.

I think both make sense.

From 010e99b403ec733d50c71a7d4ef646b1b446ef07 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Wed, 15 Jun 2016 22:52:58 -0400
Subject: [PATCH 2/2] Add VACUUM (DISABLE_PAGE_SKIPPING) for emergencies.

Furthermore,
+      except when performing an aggressive vacuum, some pages may be skipped
+      in order to avoid waiting for other sessions to finish using them.

Isn't that still the case? We'll not wait for a cleanup lock and such,
if not necessary?

/* non-export function prototypes */
-static void lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
-			   Relation *Irel, int nindexes, bool aggressive);
+static void lazy_scan_heap(Relation onerel, int options,
+			   LVRelStats *vacrelstats, Relation *Irel, int nindexes,
+			   bool aggressive);

Generally I think it's better to pass bitmaks arguments as an unsigned
integer. But since other related routines already use int...

/*
-	 * We request an aggressive scan if either the table's frozen Xid is now
+	 * We request an aggressive scan if the table's frozen Xid is now
* older than or equal to the requested Xid full-table scan limit; or if
* the table's minimum MultiXactId is older than or equal to the requested
-	 * mxid full-table scan limit.
+	 * mxid full-table scan limit; or if DISABLE_PAGE_SKIPPING was specified.
*/
aggressive = TransactionIdPrecedesOrEquals(onerel->rd_rel->relfrozenxid,
xidFullScanLimit);
aggressive |= MultiXactIdPrecedesOrEquals(onerel->rd_rel->relminmxid,
mxactFullScanLimit);
+	if (options & VACOPT_DISABLE_PAGE_SKIPPING)
+		aggressive = true;

I'm inclined to convert the agressive |= to an if, it looks a bit
jarring if consecutive assignments use differing forms.

static void
-lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
+lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
Relation *Irel, int nindexes, bool aggressive)
{
BlockNumber nblocks,
@@ -542,25 +545,28 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
* the last page.  This is worth avoiding mainly because such a lock must
* be replayed on any hot standby, where it can be disruptive.
*/
-	for (next_unskippable_block = 0;
-		 next_unskippable_block < nblocks;
-		 next_unskippable_block++)
+	next_unskippable_block = 0;
+	if ((options & VACOPT_DISABLE_PAGE_SKIPPING) == 0)
{
-		uint8		vmstatus;
-
-		vmstatus = visibilitymap_get_status(onerel, next_unskippable_block,
-											&vmbuffer);
-		if (aggressive)
+		while (next_unskippable_block < nblocks)
{
-			if ((vmstatus & VISIBILITYMAP_ALL_FROZEN) == 0)
-				break;
-		}
-		else
-		{
-			if ((vmstatus & VISIBILITYMAP_ALL_VISIBLE) == 0)
-				break;
+			uint8		vmstatus;
+
+			vmstatus = visibilitymap_get_status(onerel, next_unskippable_block,
+												&vmbuffer);
+			if (aggressive)
+			{
+				if ((vmstatus & VISIBILITYMAP_ALL_FROZEN) == 0)
+					break;
+			}
+			else
+			{
+				if ((vmstatus & VISIBILITYMAP_ALL_VISIBLE) == 0)
+					break;
+			}
+			vacuum_delay_point();
+			next_unskippable_block++;
}
-		vacuum_delay_point();
}

if (next_unskippable_block >= SKIP_PAGES_THRESHOLD)
@@ -594,26 +600,29 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
if (blkno == next_unskippable_block)
{
/* Time to advance next_unskippable_block */
-			for (next_unskippable_block++;
-				 next_unskippable_block < nblocks;
-				 next_unskippable_block++)
+			next_unskippable_block++;
+			if ((options & VACOPT_DISABLE_PAGE_SKIPPING) == 0)
{
-				uint8		vmskipflags;
-
-				vmskipflags = visibilitymap_get_status(onerel,
-													   next_unskippable_block,
-													   &vmbuffer);
-				if (aggressive)
+				while (next_unskippable_block < nblocks)
{
-					if ((vmskipflags & VISIBILITYMAP_ALL_FROZEN) == 0)
-						break;
-				}
-				else
-				{
-					if ((vmskipflags & VISIBILITYMAP_ALL_VISIBLE) == 0)
-						break;
+					uint8		vmskipflags;
+
+					vmskipflags = visibilitymap_get_status(onerel,
+													  next_unskippable_block,
+														   &vmbuffer);
+					if (aggressive)
+					{
+						if ((vmskipflags & VISIBILITYMAP_ALL_FROZEN) == 0)
+							break;
+					}
+					else
+					{
+						if ((vmskipflags & VISIBILITYMAP_ALL_VISIBLE) == 0)
+							break;
+					}
+					vacuum_delay_point();
+					next_unskippable_block++;
}
-				vacuum_delay_point();
}

This code really makes me unhappy, everytime I see it.

+			| IDENT
+				{
+					if (strcmp($1, "disable_page_skipping") == 0)
+						$$ = VACOPT_DISABLE_PAGE_SKIPPING;
+					else
+						ereport(ERROR,
+								(errcode(ERRCODE_SYNTAX_ERROR),
+							 errmsg("unrecognized VACUUM option \"%s\"", $1),
+									 parser_errposition(@1)));
+				}
;

If we could get rid of that indentation behaviour by pgindent, I'd be
eclectic.

/*
+ * Remove the visibility map fork for a relation.  If there turn out to be
+ * any bugs in the visibility map code that require rebuilding the VM, this
+ * provides users with a way to do it that is cleaner than shutting down the
+ * server and removing files by hand.
+ *
+ * This is a cut-down version of RelationTruncate.
+ */
+Datum
+pg_truncate_visibility_map(PG_FUNCTION_ARGS)
+{
+	Oid			relid = PG_GETARG_OID(0);
+	Relation	rel;
+
+	rel = relation_open(relid, AccessExclusiveLock);
+
+	if (rel->rd_rel->relkind != RELKIND_RELATION &&
+		rel->rd_rel->relkind != RELKIND_MATVIEW &&
+		rel->rd_rel->relkind != RELKIND_TOASTVALUE)
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+		   errmsg("\"%s\" is not a table, materialized view, or TOAST table",
+				  RelationGetRelationName(rel))));
+
+	RelationOpenSmgr(rel);
+	rel->rd_smgr->smgr_vm_nblocks = InvalidBlockNumber;
+
+	visibilitymap_truncate(rel, 0);
+
+	if (RelationNeedsWAL(rel))
+	{
+		xl_smgr_truncate	xlrec;
+
+		xlrec.blkno = 0;
+		xlrec.rnode = rel->rd_node;
+		xlrec.flags = SMGR_TRUNCATE_VM;
+
+		XLogBeginInsert();
+		XLogRegisterData((char *) &xlrec, sizeof(xlrec));
+
+		XLogInsert(RM_SMGR_ID, XLOG_SMGR_TRUNCATE | XLR_SPECIAL_REL_UPDATE);
+	}

Having an XLogInsert() in contrib makes me more than a bit squeamish. I
think it'd be fair bit better to have that section of code in
visibilitymap.c, and then call that from the extension.

+	/* Don't keep the relation locked any longer than necessary! */
+	relation_close(rel, AccessExclusiveLock);

Don't think that's a good idea. We use transactional semantics for a
good reason, and the function returns afterwards anyway.

I'm also thinking that we should perform owner-checks in these
functions. Sure, by default they're superuser only. But there's
legitimate reason to grant execute to other users, but that shouldn't
automatically mean they can do everything. That's probably done best in
a separate commit tho.

+/* flags for xl_smgr_truncate */
+#define SMGR_TRUNCATE_HEAP		0x0001
+#define SMGR_TRUNCATE_VM		0x0002
+#define SMGR_TRUNCATE_FSM		0x0004
+#define SMGR_TRUNCATE_ALL		\
+	(SMGR_TRUNCATE_HEAP|SMGR_TRUNCATE_VM|SMGR_TRUNCATE_FSM)
+

Hm. I wonder if somebody will forget to update this the next time a new
fork is introduced...

- Andres

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

On Fri, Jun 17, 2016 at 2:48 PM, Andres Freund <andres@anarazel.de> wrote:

From 010e99b403ec733d50c71a7d4ef646b1b446ef07 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Wed, 15 Jun 2016 22:52:58 -0400
Subject: [PATCH 2/2] Add VACUUM (DISABLE_PAGE_SKIPPING) for emergencies.

Furthermore,
+      except when performing an aggressive vacuum, some pages may be skipped
+      in order to avoid waiting for other sessions to finish using them.

Isn't that still the case? We'll not wait for a cleanup lock and such,
if not necessary?

That's just there to explain what behavior gets changed when you
specify DISABLE_PAGE_SKIPPING.

/* non-export function prototypes */
-static void lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
-                        Relation *Irel, int nindexes, bool aggressive);
+static void lazy_scan_heap(Relation onerel, int options,
+                        LVRelStats *vacrelstats, Relation *Irel, int nindexes,
+                        bool aggressive);

Generally I think it's better to pass bitmaks arguments as an unsigned
integer. But since other related routines already use int...

Many years ago, I was told by Tom Lane that project standard was int.
My gut was to use unsigned too, but I've got better things to do than
argue about it - and project style is a legitimate argument in any
case.

/*
-      * We request an aggressive scan if either the table's frozen Xid is now
+      * We request an aggressive scan if the table's frozen Xid is now
* older than or equal to the requested Xid full-table scan limit; or if
* the table's minimum MultiXactId is older than or equal to the requested
-      * mxid full-table scan limit.
+      * mxid full-table scan limit; or if DISABLE_PAGE_SKIPPING was specified.
*/
aggressive = TransactionIdPrecedesOrEquals(onerel->rd_rel->relfrozenxid,
xidFullScanLimit);
aggressive |= MultiXactIdPrecedesOrEquals(onerel->rd_rel->relminmxid,
mxactFullScanLimit);
+     if (options & VACOPT_DISABLE_PAGE_SKIPPING)
+             aggressive = true;

I'm inclined to convert the agressive |= to an if, it looks a bit
jarring if consecutive assignments use differing forms.

I thought about that, but I like it better the way I have it.

This code really makes me unhappy, everytime I see it.

That's not a defect in this patch.

+                     | IDENT
+                             {
+                                     if (strcmp($1, "disable_page_skipping") == 0)
+                                             $$ = VACOPT_DISABLE_PAGE_SKIPPING;
+                                     else
+                                             ereport(ERROR,
+                                                             (errcode(ERRCODE_SYNTAX_ERROR),
+                                                      errmsg("unrecognized VACUUM option \"%s\"", $1),
+                                                                      parser_errposition(@1)));
+                             }
;

If we could get rid of that indentation behaviour by pgindent, I'd be
eclectic.

Neither is that, although the indentation there looks fine to me.

/*
+ * Remove the visibility map fork for a relation.  If there turn out to be
+ * any bugs in the visibility map code that require rebuilding the VM, this
+ * provides users with a way to do it that is cleaner than shutting down the
+ * server and removing files by hand.
+ *
+ * This is a cut-down version of RelationTruncate.
+ */
+Datum
+pg_truncate_visibility_map(PG_FUNCTION_ARGS)
+{
+     Oid                     relid = PG_GETARG_OID(0);
+     Relation        rel;
+
+     rel = relation_open(relid, AccessExclusiveLock);
+
+     if (rel->rd_rel->relkind != RELKIND_RELATION &&
+             rel->rd_rel->relkind != RELKIND_MATVIEW &&
+             rel->rd_rel->relkind != RELKIND_TOASTVALUE)
+             ereport(ERROR,
+                             (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                errmsg("\"%s\" is not a table, materialized view, or TOAST table",
+                               RelationGetRelationName(rel))));
+
+     RelationOpenSmgr(rel);
+     rel->rd_smgr->smgr_vm_nblocks = InvalidBlockNumber;
+
+     visibilitymap_truncate(rel, 0);
+
+     if (RelationNeedsWAL(rel))
+     {
+             xl_smgr_truncate        xlrec;
+
+             xlrec.blkno = 0;
+             xlrec.rnode = rel->rd_node;
+             xlrec.flags = SMGR_TRUNCATE_VM;
+
+             XLogBeginInsert();
+             XLogRegisterData((char *) &xlrec, sizeof(xlrec));
+
+             XLogInsert(RM_SMGR_ID, XLOG_SMGR_TRUNCATE | XLR_SPECIAL_REL_UPDATE);
+     }

Having an XLogInsert() in contrib makes me more than a bit squeamish. I
think it'd be fair bit better to have that section of code in
visibilitymap.c, and then call that from the extension.

I thought about that too, but it seemed like it was just bloating the
core server for no real reason. It's not like contrib is off in space
someplace.

+     /* Don't keep the relation locked any longer than necessary! */
+     relation_close(rel, AccessExclusiveLock);

Don't think that's a good idea. We use transactional semantics for a
good reason, and the function returns afterwards anyway.

Yeah, but if you want to blow away a bunch of visibility map forks at
one go, you're not going to appreciate this function collecting all of
those locks at the same time. This is also why VACUUM starts a
separate transaction for each table.

I'm also thinking that we should perform owner-checks in these
functions. Sure, by default they're superuser only. But there's
legitimate reason to grant execute to other users, but that shouldn't
automatically mean they can do everything. That's probably done best in
a separate commit tho.

I think it is a very dubious idea to grant access to these functions
to anyone other than the superuser, but if you want to add an owner
check, go for it!

+/* flags for xl_smgr_truncate */
+#define SMGR_TRUNCATE_HEAP           0x0001
+#define SMGR_TRUNCATE_VM             0x0002
+#define SMGR_TRUNCATE_FSM            0x0004
+#define SMGR_TRUNCATE_ALL            \
+     (SMGR_TRUNCATE_HEAP|SMGR_TRUNCATE_VM|SMGR_TRUNCATE_FSM)
+

Hm. I wonder if somebody will forget to update this the next time a new
fork is introduced...

Gosh, I hope we go for getting rid of some of these forks instead of
adding more, but yeah, that could possibly happen. I don't think it's
a big cause for concern, though.

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

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

Since time is short and it seems better to get the xlog page magic
bump done before beta2, I've gone ahead and committed this, but there
are two points here that seem to warrant some input from other senior
hackers. Andres and I have discussed these points off list but
without reaching a meeting of the minds.

On Fri, Jun 17, 2016 at 2:59 PM, Robert Haas <robertmhaas@gmail.com> wrote:

Having an XLogInsert() in contrib makes me more than a bit squeamish. I
think it'd be fair bit better to have that section of code in
visibilitymap.c, and then call that from the extension.

I thought about that too, but it seemed like it was just bloating the
core server for no real reason. It's not like contrib is off in space
someplace.

So, Andres thinks it's a bad idea to have an XLogInsert() call in
contrib, and I don't think it matters. Anyone else have an opinion?
Andres, do you want to explain the nature of your concern further?

+     /* Don't keep the relation locked any longer than necessary! */
+     relation_close(rel, AccessExclusiveLock);

Don't think that's a good idea. We use transactional semantics for a
good reason, and the function returns afterwards anyway.

Yeah, but if you want to blow away a bunch of visibility map forks at
one go, you're not going to appreciate this function collecting all of
those locks at the same time. This is also why VACUUM starts a
separate transaction for each table.

Andres has a concern here that there might be a problem related to
invalidation messages. Currently, the only invalidation message
that's getting generated here is a non-transactional smgr
invalidation, which is sent before the lock is released and therefore
no race exists. However, Andres is concerned that this may at a
minimum not be very future-proof and possibly unsafe even today. To
me, however, this doesn't seem particularly different than what e.g.
lazy_truncate_heap() always does and has done for a long time. More
input here is welcome and appreciated - it would be good not to screw
this up.

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

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

On Sat, Jun 18, 2016 at 6:53 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Jun 17, 2016 at 2:59 PM, Robert Haas <robertmhaas@gmail.com> wrote:

Having an XLogInsert() in contrib makes me more than a bit squeamish. I
think it'd be fair bit better to have that section of code in
visibilitymap.c, and then call that from the extension.

I thought about that too, but it seemed like it was just bloating the
core server for no real reason. It's not like contrib is off in space
someplace.

So, Andres thinks it's a bad idea to have an XLogInsert() call in
contrib, and I don't think it matters. Anyone else have an opinion?

I am meh as well with this practice that we should not encourage. xlog
insertion routines should just be generated by in-core code. We have
now the generic WAL interface for extensions, and if need be it should
be extended.

Andres, do you want to explain the nature of your concern further?

I am not in his mind, but my guess is that contrib modules are
sometimes used as template examples by other people, and encouraging
users to use those routines in modules would increase the risk to
misuse them, aka badly-formed records that could corrupt the system.
--
Michael

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

Michael Paquier <michael.paquier@gmail.com> writes:

On Sat, Jun 18, 2016 at 6:53 AM, Robert Haas <robertmhaas@gmail.com> wrote:

Andres, do you want to explain the nature of your concern further?

I am not in his mind, but my guess is that contrib modules are
sometimes used as template examples by other people, and encouraging
users to use those routines in modules would increase the risk to
misuse them, aka badly-formed records that could corrupt the system.

I don't follow that argument. People writing new extensions are just
as likely to copy from core code as contrib.

If Andres' concern is that XLogInsert isn't a very stable API, maybe
we could address that by providing some wrapper function that knows
how to emit the specific kind of record that pg_visibility needs.
But on the whole it seems like make-work, unless there's a reason
to believe that other extensions will need to generate that exact
same record type.

regards, tom lane

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

On 2016-06-18 11:56:51 -0400, Tom Lane wrote:

Michael Paquier <michael.paquier@gmail.com> writes:

On Sat, Jun 18, 2016 at 6:53 AM, Robert Haas <robertmhaas@gmail.com> wrote:

Andres, do you want to explain the nature of your concern further?

I am not in his mind, but my guess is that contrib modules are
sometimes used as template examples by other people, and encouraging
users to use those routines in modules would increase the risk to
misuse them, aka badly-formed records that could corrupt the system.

That's not it, no.

If Andres' concern is that XLogInsert isn't a very stable API, maybe
we could address that by providing some wrapper function that knows
how to emit the specific kind of record that pg_visibility needs.

That's part of the concern I have, yes. It's pretty common that during
minor version updates contrib modules are updated before the main server
is restarted. Increasing the coupling on something as critical as WAL
logging doesn't strike me as a good idea.

I also don't see why it's a good idea to have knowledge about how to
truncate the visibility map outside of visibilitymap.c. Having that in a
contrib module just seems like a modularity violation. To me this
should be a wal_log paramter to visibilitymap_truncate().

Andres

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

Andres Freund <andres@anarazel.de> writes:

I also don't see why it's a good idea to have knowledge about how to
truncate the visibility map outside of visibilitymap.c. Having that in a
contrib module just seems like a modularity violation.

That seems like a pretty good argument.

regards, tom lane

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

On Mon, Jun 20, 2016 at 4:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

I also don't see why it's a good idea to have knowledge about how to
truncate the visibility map outside of visibilitymap.c. Having that in a
contrib module just seems like a modularity violation.

That seems like a pretty good argument.

I agree that's a good argument but it passes over one or two points
which motivated my approach. Most of the work of truncating the
visibility map is, in fact, encapsulated by visibilitymap_truncate, so
the only knowledge we're exporting to the contrib module is what WAL
record to emit. I put that in the caller of visibilitymap_truncate
because the only existing caller of visibilitymap_truncate is
RelationTruncate, which also knows what WAL record to emit on behalf
of visibilitymap.c. So I don't think I've exported any more knowledge
from visibilitymap.c than was the case previously.

That having been said, if somebody wants to whack this around, I am
not going to put up a big fight.

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

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