Date: Mon, 10 Aug 2020 14:24:12 -0400
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Michael Paquier <michael@paquier.xyz>, exclusion@gmail.com,
	pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16577: Segfault on altering a table located in a dropped
 tablespace
Message-ID: <20200810182412.GA19588@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="J2SCkAp4GZ/dPZZf"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <3064988.1596991985@sss.pgh.pa.us>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk


--J2SCkAp4GZ/dPZZf
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2020-Aug-09, Tom Lane wrote:

> Michael Paquier <michael@paquier.xyz> writes:
> > Thanks Alexander for the report.  Interesting case indeed.
> > For a normal table we would complain that the tablespace is not empty
> > when attempting to drop the tablespace.  But here we have only one
> > partitioned table still holding references to the tablespace.

Ah, so it turns out that the physical files were necessary after all.
Maybe the solution to this problem is indeed to have them.  It means
partly reverting this commit:

commit 807ae415c54628ade937cb209f0fc9913e6b0cf5
Author:     Alvaro Herrera <alvherre@alvh.no-ip.org>
AuthorDate: Fri Jan 4 14:51:17 2019 -0300
CommitDate: Fri Jan 4 14:51:17 2019 -0300

    Don't create relfilenode for relations without storage
    
    Some relation kinds had relfilenode set to some non-zero value, but
    apparently the actual files did not really exist because creation was
    prevented elsewhere.  Get rid of the phony pg_class.relfilenode values.
    
    Catversion bumped, but only because the sanity_test check will fail if
    run in a system initdb'd with the previous version.
    
    Reviewed-by: Kyotaro HORIGUCHI, Michael Paquier
    Discussion: https://postgr.es/m/20181206215552.fm2ypuxq6nhpwjuc@alvherre.pgsql

But also fixing whatever *other* code was preventing creating of the
filenodes.

As for the crash at hand, it seems it can be solved easily by making
ruleutils avoid trying to dereference a null pointer, as in the attached
patch.  This seems necessary for branches 12 and 13, which have the
above commit, but not for 10/11 (which do have relfilenodes) nor master,
where we can revert it.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--J2SCkAp4GZ/dPZZf
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="part-idx-tblspc.patch"

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 2cbcb4b85e..d6c4b5eb0c 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -1451,14 +1451,24 @@ pg_get_indexdef_worker(Oid indexrelid, int colno,
 		if (showTblSpc)
 		{
 			Oid			tblspc;
+			char	   *spcname = NULL;
 
+			/*
+			 * If this is an index on a partitioned table, it's possible that
+			 * the tablespace was dropped, because there are insufficient
+			 * protections in that case.  Don't add anything if so (and also
+			 * don't crash).
+			 */
 			tblspc = get_rel_tablespace(indexrelid);
-			if (OidIsValid(tblspc))
+			if (tblspc != InvalidOid)
+				spcname = get_tablespace_name(tblspc);
+			if (spcname != NULL)
 			{
 				if (isConstraint)
 					appendStringInfoString(&buf, " USING INDEX");
 				appendStringInfo(&buf, " TABLESPACE %s",
-								 quote_identifier(get_tablespace_name(tblspc)));
+								 quote_identifier(spcname));
+				pfree(spcname);
 			}
 		}
 
@@ -2188,6 +2198,7 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand,
 				{
 					char	   *options = flatten_reloptions(indexId);
 					Oid			tblspc;
+					char	   *spcname = NULL;
 
 					if (options)
 					{
@@ -2200,11 +2211,18 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand,
 					 * This is to help ALTER TABLE usage of this facility,
 					 * which needs this behavior to recreate exact catalog
 					 * state.
+					 *
+					 * If this is an index on a partitioned table, it's
+					 * possible that the tablespace was dropped, because there
+					 * are insufficient protections in that case.  Don't add
+					 * anything if so (and also don't crash).
 					 */
 					tblspc = get_rel_tablespace(indexId);
-					if (OidIsValid(tblspc))
+					if (tblspc != InvalidOid)
+						spcname = get_tablespace_name(tblspc);
+					if (spcname != NULL)
 						appendStringInfo(&buf, " USING INDEX TABLESPACE %s",
-										 quote_identifier(get_tablespace_name(tblspc)));
+								quote_identifier(spcname));
 				}
 
 				break;

--J2SCkAp4GZ/dPZZf--