On Mon, Apr 8, 2024 at 9:54 PM Robert Haas <robertmh...@gmail.com> wrote:
> On Mon, Apr 8, 2024 at 12:33 PM Alexander Korotkov <aekorot...@gmail.com> 
> wrote:
> > Yes, it was my mistake. I got rushing trying to fit this to FF, even doing 
> > significant changes just before commit.
> > I'll revert this later today.

The patch to revert is attached.  Given that revert touches the work
done in 041b96802e, I think it needs some feedback before push.

> Alexander,
>
> Exactly how much is getting reverted here? I see these, all since March 23rd:
>
> dd1f6b0c17 Provide a way block-level table AMs could re-use
> acquire_sample_rows()
> 9bd99f4c26 Custom reloptions for table AM
> 97ce821e3e Fix the parameters order for
> TableAmRoutine.relation_copy_for_cluster()
> 867cc7b6dd Revert "Custom reloptions for table AM"
> b1484a3f19 Let table AM insertion methods control index insertion
> c95c25f9af Custom reloptions for table AM
> 27bc1772fc Generalize relation analyze in table AM interface
> 87985cc925 Allow locking updated tuples in tuple_update() and tuple_delete()
> c35a3fb5e0 Allow table AM tuple_insert() method to return the different slot
> 02eb07ea89 Allow table AM to store complex data structures in rd_amcache
>
> I'm not really feeling very good about all of this, because:
>
> - 87985cc925 was previously committed as 11470f544e on March 23, 2023,
> and almost immediately reverted. Now you tried again on March 26,
> 2024. I know there was a bunch of rework in the middle, but there are
> times in the year that things can be committed other than right before
> the feature freeze. Like, don't wait a whole year for the next attempt
> and then again do it right before the cutoff.

I agree with the facts.  But I have a different interpretation on
this.  The patch was committed as 11470f544e on March 23, 2023, then
reverted on April 3.  I've proposed the revised version, but Andres
complained that this is the new API design days before FF.  Then the
patch with this design was published in the thread for the year with
periodical rebases.  So, I think I expressed my intention with that
design before 2023 FF, nobody prevented me from expressing objections
or other feedback during the year.  Then I realized that 2024 FF is
approaching and decided to give this another try for pg18.

But I don't yet see it's wrong with this patch.  I waited a year for
feedback.  I waited 2 days after saying "I will push this if no
objections". Given your feedback now, I get that it would be better to
do another attempt to commit this earlier.

I admit my mistake with dd1f6b0c17.  I get rushed trying to fix the
things actually making things worse.  I apologise for this.  But if
I'm forced to revert 87985cc925 without even hearing any reasonable
critics besides imperfection of timing, I feel like this is the
punishment for my mistake with dd1f6b0c17.  Pretty unreasonable
punishment in my view.

> - The Discussion links in the commit messages do not seem to stand for
> the proposition that these particular patches ought to be committed in
> this form. Some of them are just links to the messages where the patch
> was originally posted, which is probably not against policy or
> anything, but it'd be nicer to see links to versions of the patch with
> which people are, in nearby emails, agreeing. Even worse, some of
> these are links to emails where somebody said, "hey, some earlier
> commit does not look good." In particular,
> dd1f6b0c172a643a73d6b71259fa2d10378b39eb has a discussion link where
> Andres complains about 27bc1772fc814946918a5ac8ccb9b5c5ad0380aa, but
> it's not clear how that justifies the new commit.

I have to repeat again, that I admit my mistake with dd1f6b0c17,
apologize for that, and make my own conclusions to not repeat this.
But dd1f6b0c17 seems to be the only one that has a link to the message
with complains.  I went through the list of commits above, it seems
that others have just linked to the first message of the thread.
Probably, there is a lack of consensus for some of them.  But I never
heard about a policy to link not just the discussion start, but also
exact messages expressing agreeing.  And I didn't see others doing
that.

> - The commit message for 867cc7b6dd says "This reverts commit
> c95c25f9af4bc77f2f66a587735c50da08c12b37 due to multiple design issues
> spotted after commit." That's not a very good justification for then
> trying again 6 days later with 9bd99f4c26, and it's *definitely* not a
> good justification for there being no meaningful discussion links in
> the commit message for 9bd99f4c26. They're just the same links you had
> in the previous attempt, so it's pretty hard for anybody to understand
> what got fixed and whether all of the concerns were really addressed.
> Just looking over the commit, it's pretty hard to understand what is
> being changed and why: there's not a lot of comment updates, there's
> no documentation changes, and there's not a lot of explanation in the
> commit message either. Even if this feature is great and all the code
> is perfect now, it's going to be hard for anyone to figure out how to
> use it.

1) 9bd99f4c26 comprises the reworked patch after working with notes
from Jeff Davis.  I agree it would be better to wait for him to
express explicit agreement.  Before reverting this, I would prefer to
hear his opinion.
2) One of the issues here is that table AM API doesn't have
documentation, it has just a very brief page which doesn't go deep
explaining particular API methods.  I have heard a lot of complains
about that from users attempting to write table access methods.  It's
now too late to complain about that (but if I had a wisdom of now back
during pg12 development I would definitely object against table AM API
being committed at that shape).  I understand I could be more
proactive and propose a patch with that documentation.

> 97ce821e3e looks like a clear bug fix to me, but I wonder if the rest
> of this should all just be reverted, with a ban on ever trying it
> again after March 1 of any year.

Do you propose a ban from March 1 to the end of any year?  I think the
first doesn't make sense, because it leaves only 2 months a year for
the work.  That would create a potential rush during these 2 month and
could serve exactly opposite to the intention.  So, I guess this means
a ban from March 1 to the FF of any year.  The situation now is quite
unpleasant for me.  So I'm far from repeating this next year.
However, if there should be a formal ban, it should be specified.
Does it relate to the patches I've pushed, all patches in this thread,
all similar patches, all table AM patches, or other API patches?

Surely, I'm an interested party and can't be impartial.  But I think
it would be nice if we introduce some general rules based on this
experience.  Could we have some API freeze date some time before the
feature freeze?

> I'd like to believe that there are
> only bookkeeping problems here, and that there was in fact clear
> agreement that all of these changes should be made in this form, and
> that the commit messages simply failed to reference the most relevant
> emails. But what I fear, especially in view of Andres's remarks, is
> that these commits were done in haste without adequate consensus, and
> I think that's a serious problem.

This thread had a lot of patches for table AM API.  My intention for
pg17 was to commit the easiest and least contradictory of them.  I
understand there should be more consensus for some of them and
committing dd1f6b0c17 instead of reverting 27bc1772fc was a mistake.
But I don't feel good about reverting everything in a row without
clear feedback.

------
Regards,
Alexander Korotkov
From ba472259f26ec0894fbe08b40ac7a23cdb3abf61 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Wed, 10 Apr 2024 11:58:46 +0300
Subject: [PATCH v1] revert: Generalize relation analyze in table AM interface

This commit reverts 27bc1772fc and dd1f6b0c17.  The way a block-level table
AMs could re-use acquire_sample_rows() didn't get enough of review.

Discussion: https://postgr.es/m/20240408160851.drpi33adgxhcb2oa%40awork3.anarazel.de
---
 src/backend/access/heap/heapam_handler.c |  30 ++-----
 src/backend/access/table/tableamapi.c    |   2 +
 src/backend/commands/analyze.c           |  55 ++++--------
 src/include/access/heapam.h              |   8 --
 src/include/access/tableam.h             | 101 ++++++++++++++++++-----
 src/include/commands/vacuum.h            |  69 ----------------
 src/include/foreign/fdwapi.h             |   8 +-
 src/tools/pgindent/typedefs.list         |   3 -
 8 files changed, 108 insertions(+), 168 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 30095d88b09..2485dfa036b 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -52,6 +52,7 @@ static TM_Result heapam_tuple_lock(Relation relation, ItemPointer tid,
 								   CommandId cid, LockTupleMode mode,
 								   LockWaitPolicy wait_policy, uint8 flags,
 								   TM_FailureData *tmfd);
+
 static void reform_and_rewrite_tuple(HeapTuple tuple,
 									 Relation OldHeap, Relation NewHeap,
 									 Datum *values, bool *isnull, RewriteState rwstate);
@@ -1064,7 +1065,7 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap,
  * until heapam_scan_analyze_next_tuple() returns false.  That is until all the
  * items of the heap page are analyzed.
  */
-bool
+static bool
 heapam_scan_analyze_next_block(TableScanDesc scan, ReadStream *stream)
 {
 	HeapScanDesc hscan = (HeapScanDesc) scan;
@@ -1088,17 +1089,7 @@ heapam_scan_analyze_next_block(TableScanDesc scan, ReadStream *stream)
 	return true;
 }
 
-/*
- * Iterate over tuples in the block selected with
- * heapam_scan_analyze_next_block().  If a tuple that's suitable for sampling
- * is found, true is returned and a tuple is stored in `slot`.  When no more
- * tuples for sampling, false is returned and the pin and lock acquired by
- * heapam_scan_analyze_next_block() are released.
- *
- * *liverows and *deadrows are incremented according to the encountered
- * tuples.
- */
-bool
+static bool
 heapam_scan_analyze_next_tuple(TableScanDesc scan, TransactionId OldestXmin,
 							   double *liverows, double *deadrows,
 							   TupleTableSlot *slot)
@@ -2666,18 +2657,6 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer,
 	}
 }
 
-/*
- * heapap_analyze -- implementation of relation_analyze() for heap
- *					 table access method
- */
-static void
-heapam_analyze(Relation relation, AcquireSampleRowsFunc *func,
-			   BlockNumber *totalpages, BufferAccessStrategy bstrategy)
-{
-	block_level_table_analyze(relation, func, totalpages, bstrategy,
-							  heapam_scan_analyze_next_block,
-							  heapam_scan_analyze_next_tuple);
-}
 
 /* ------------------------------------------------------------------------
  * Definition of the heap table access method.
@@ -2725,9 +2704,10 @@ static const TableAmRoutine heapam_methods = {
 	.relation_copy_data = heapam_relation_copy_data,
 	.relation_copy_for_cluster = heapam_relation_copy_for_cluster,
 	.relation_vacuum = heap_vacuum_rel,
+	.scan_analyze_next_block = heapam_scan_analyze_next_block,
+	.scan_analyze_next_tuple = heapam_scan_analyze_next_tuple,
 	.index_build_range_scan = heapam_index_build_range_scan,
 	.index_validate_scan = heapam_index_validate_scan,
-	.relation_analyze = heapam_analyze,
 
 	.free_rd_amcache = NULL,
 	.relation_size = table_block_relation_size,
diff --git a/src/backend/access/table/tableamapi.c b/src/backend/access/table/tableamapi.c
index d9e23ef3175..c1feef43d8e 100644
--- a/src/backend/access/table/tableamapi.c
+++ b/src/backend/access/table/tableamapi.c
@@ -83,6 +83,8 @@ GetTableAmRoutine(Oid amhandler)
 	Assert(routine->relation_copy_data != NULL);
 	Assert(routine->relation_copy_for_cluster != NULL);
 	Assert(routine->relation_vacuum != NULL);
+	Assert(routine->scan_analyze_next_block != NULL);
+	Assert(routine->scan_analyze_next_tuple != NULL);
 	Assert(routine->index_build_range_scan != NULL);
 	Assert(routine->index_validate_scan != NULL);
 
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 516b43b0e34..7d2cd249972 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -17,7 +17,6 @@
 #include <math.h>
 
 #include "access/detoast.h"
-#include "access/heapam.h"
 #include "access/genam.h"
 #include "access/multixact.h"
 #include "access/relation.h"
@@ -76,8 +75,6 @@ int			default_statistics_target = 100;
 /* A few variables that don't seem worth passing around as parameters */
 static MemoryContext anl_context = NULL;
 static BufferAccessStrategy vac_strategy;
-static ScanAnalyzeNextBlockFunc scan_analyze_next_block;
-static ScanAnalyzeNextTupleFunc scan_analyze_next_tuple;
 
 
 static void do_analyze_rel(Relation onerel,
@@ -90,6 +87,9 @@ static void compute_index_stats(Relation onerel, double totalrows,
 								MemoryContext col_context);
 static VacAttrStats *examine_attribute(Relation onerel, int attnum,
 									   Node *index_expr);
+static int	acquire_sample_rows(Relation onerel, int elevel,
+								HeapTuple *rows, int targrows,
+								double *totalrows, double *totaldeadrows);
 static int	compare_rows(const void *a, const void *b, void *arg);
 static int	acquire_inherited_sample_rows(Relation onerel, int elevel,
 										  HeapTuple *rows, int targrows,
@@ -190,12 +190,10 @@ analyze_rel(Oid relid, RangeVar *relation,
 	if (onerel->rd_rel->relkind == RELKIND_RELATION ||
 		onerel->rd_rel->relkind == RELKIND_MATVIEW)
 	{
-		/*
-		 * Get row acquisition function, blocks and tuples iteration callbacks
-		 * provided by table AM
-		 */
-		table_relation_analyze(onerel, &acquirefunc,
-							   &relpages, vac_strategy);
+		/* Regular table, so we'll use the regular row acquisition function */
+		acquirefunc = acquire_sample_rows;
+		/* Also get regular table's size */
+		relpages = RelationGetNumberOfBlocks(onerel);
 	}
 	else if (onerel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
 	{
@@ -1119,17 +1117,15 @@ block_sampling_read_stream_next(ReadStream *stream,
 }
 
 /*
- * acquire_sample_rows -- acquire a random sample of rows from the
- * block-based relation
+ * acquire_sample_rows -- acquire a random sample of rows from the table
  *
  * Selected rows are returned in the caller-allocated array rows[], which
  * must have at least targrows entries.
  * The actual number of rows selected is returned as the function result.
- * We also estimate the total numbers of live and dead rows in the relation,
+ * We also estimate the total numbers of live and dead rows in the table,
  * and return them into *totalrows and *totaldeadrows, respectively.
  *
- * The returned list of tuples is in order by physical position in the
- * relation.
+ * The returned list of tuples is in order by physical position in the table.
  * (We will rely on this later to derive correlation estimates.)
  *
  * As of May 2004 we use a new two-stage method:  Stage one selects up
@@ -1151,7 +1147,7 @@ block_sampling_read_stream_next(ReadStream *stream,
  * look at a statistically unbiased set of blocks, we should get
  * unbiased estimates of the average numbers of live and dead rows per
  * block.  The previous sampling method put too much credence in the row
- * density near the start of the relation.
+ * density near the start of the table.
  */
 static int
 acquire_sample_rows(Relation onerel, int elevel,
@@ -1204,11 +1200,11 @@ acquire_sample_rows(Relation onerel, int elevel,
 										0);
 
 	/* Outer loop over blocks to sample */
-	while (scan_analyze_next_block(scan, stream))
+	while (table_scan_analyze_next_block(scan, stream))
 	{
 		vacuum_delay_point();
 
-		while (scan_analyze_next_tuple(scan, OldestXmin, &liverows, &deadrows, slot))
+		while (table_scan_analyze_next_tuple(scan, OldestXmin, &liverows, &deadrows, slot))
 		{
 			/*
 			 * The first targrows sample rows are simply copied into the
@@ -1331,25 +1327,6 @@ compare_rows(const void *a, const void *b, void *arg)
 	return 0;
 }
 
-/*
- * block_level_table_analyze -- implementation of relation_analyze() for
- *								block-level table access methods
- */
-void
-block_level_table_analyze(Relation relation,
-						  AcquireSampleRowsFunc *func,
-						  BlockNumber *totalpages,
-						  BufferAccessStrategy bstrategy,
-						  ScanAnalyzeNextBlockFunc scan_analyze_next_block_cb,
-						  ScanAnalyzeNextTupleFunc scan_analyze_next_tuple_cb)
-{
-	*func = acquire_sample_rows;
-	*totalpages = RelationGetNumberOfBlocks(relation);
-	vac_strategy = bstrategy;
-	scan_analyze_next_block = scan_analyze_next_block_cb;
-	scan_analyze_next_tuple = scan_analyze_next_tuple_cb;
-}
-
 
 /*
  * acquire_inherited_sample_rows -- acquire sample rows from inheritance tree
@@ -1439,9 +1416,9 @@ acquire_inherited_sample_rows(Relation onerel, int elevel,
 		if (childrel->rd_rel->relkind == RELKIND_RELATION ||
 			childrel->rd_rel->relkind == RELKIND_MATVIEW)
 		{
-			/* Use row acquisition function provided by table AM */
-			table_relation_analyze(childrel, &acquirefunc,
-								   &relpages, vac_strategy);
+			/* Regular table, so use the regular row acquisition function */
+			acquirefunc = acquire_sample_rows;
+			relpages = RelationGetNumberOfBlocks(childrel);
 		}
 		else if (childrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
 		{
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index f84dbe629fe..be630620d0d 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -412,14 +412,6 @@ extern bool HeapTupleHeaderIsOnlyLocked(HeapTupleHeader tuple);
 extern bool HeapTupleIsSurelyDead(HeapTuple htup,
 								  struct GlobalVisState *vistest);
 
-/* in heap/heapam_handler.c*/
-extern bool heapam_scan_analyze_next_block(TableScanDesc scan,
-										   ReadStream *stream);
-extern bool heapam_scan_analyze_next_tuple(TableScanDesc scan,
-										   TransactionId OldestXmin,
-										   double *liverows, double *deadrows,
-										   TupleTableSlot *slot);
-
 /*
  * To avoid leaking too much knowledge about reorderbuffer implementation
  * details this is implemented in reorderbuffer.c not heapam_visibility.c
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index d1cd71b7a17..4e963cf41e7 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -20,8 +20,8 @@
 #include "access/relscan.h"
 #include "access/sdir.h"
 #include "access/xact.h"
-#include "commands/vacuum.h"
 #include "executor/tuptable.h"
+#include "storage/read_stream.h"
 #include "utils/rel.h"
 #include "utils/snapshot.h"
 
@@ -668,6 +668,50 @@ typedef struct TableAmRoutine
 									struct VacuumParams *params,
 									BufferAccessStrategy bstrategy);
 
+	/*
+	 * Prepare to analyze the next block in the read stream.  Returns false if
+	 * the stream is exhausted and true otherwise. The scan must have been
+	 * started with SO_TYPE_ANALYZE option.
+	 *
+	 * This routine holds a buffer pin and lock on the heap page.  They are
+	 * held until heapam_scan_analyze_next_tuple() returns false.  That is
+	 * until all the items of the heap page are analyzed.
+	 */
+
+	/*
+	 * Prepare to analyze block `blockno` of `scan`. The scan has been started
+	 * with table_beginscan_analyze().  See also
+	 * table_scan_analyze_next_block().
+	 *
+	 * The callback may acquire resources like locks that are held until
+	 * table_scan_analyze_next_tuple() returns false. It e.g. can make sense
+	 * to hold a lock until all tuples on a block have been analyzed by
+	 * scan_analyze_next_tuple.
+	 *
+	 * The callback can return false if the block is not suitable for
+	 * sampling, e.g. because it's a metapage that could never contain tuples.
+	 *
+	 * XXX: This obviously is primarily suited for block-based AMs. It's not
+	 * clear what a good interface for non block based AMs would be, so there
+	 * isn't one yet.
+	 */
+	bool		(*scan_analyze_next_block) (TableScanDesc scan,
+											ReadStream *stream);
+
+	/*
+	 * See table_scan_analyze_next_tuple().
+	 *
+	 * Not every AM might have a meaningful concept of dead rows, in which
+	 * case it's OK to not increment *deadrows - but note that that may
+	 * influence autovacuum scheduling (see comment for relation_vacuum
+	 * callback).
+	 */
+	bool		(*scan_analyze_next_tuple) (TableScanDesc scan,
+											TransactionId OldestXmin,
+											double *liverows,
+											double *deadrows,
+											TupleTableSlot *slot);
+
 	/* see table_index_build_range_scan for reference about parameters */
 	double		(*index_build_range_scan) (Relation table_rel,
 										   Relation index_rel,
@@ -688,12 +732,6 @@ typedef struct TableAmRoutine
 										Snapshot snapshot,
 										struct ValidateIndexState *state);
 
-	/* See table_relation_analyze() */
-	void		(*relation_analyze) (Relation relation,
-									 AcquireSampleRowsFunc *func,
-									 BlockNumber *totalpages,
-									 BufferAccessStrategy bstrategy);
-
 
 	/* ------------------------------------------------------------------------
 	 * Miscellaneous functions.
@@ -1766,6 +1804,40 @@ table_relation_vacuum(Relation rel, struct VacuumParams *params,
 	rel->rd_tableam->relation_vacuum(rel, params, bstrategy);
 }
 
+/*
+ * Prepare to analyze the next block in the read stream. The scan needs to
+ * have been  started with table_beginscan_analyze().  Note that this routine
+ * might acquire resources like locks that are held until
+ * table_scan_analyze_next_tuple() returns false.
+ *
+ * Returns false if block is unsuitable for sampling, true otherwise.
+ */
+static inline bool
+table_scan_analyze_next_block(TableScanDesc scan, ReadStream *stream)
+{
+	return scan->rs_rd->rd_tableam->scan_analyze_next_block(scan, stream);
+}
+
+/*
+ * Iterate over tuples in the block selected with
+ * table_scan_analyze_next_block() (which needs to have returned true, and
+ * this routine may not have returned false for the same block before). If a
+ * tuple that's suitable for sampling is found, true is returned and a tuple
+ * is stored in `slot`.
+ *
+ * *liverows and *deadrows are incremented according to the encountered
+ * tuples.
+ */
+static inline bool
+table_scan_analyze_next_tuple(TableScanDesc scan, TransactionId OldestXmin,
+							  double *liverows, double *deadrows,
+							  TupleTableSlot *slot)
+{
+	return scan->rs_rd->rd_tableam->scan_analyze_next_tuple(scan, OldestXmin,
+															liverows, deadrows,
+															slot);
+}
+
 /*
  * table_index_build_scan - scan the table to find tuples to be indexed
  *
@@ -1871,21 +1943,6 @@ table_index_validate_scan(Relation table_rel,
 											   state);
 }
 
-/*
- * table_relation_analyze - fill the infromation for a sampling statistics
- *							acquisition
- *
- * The pointer to a function that will collect sample rows from the table
- * should be stored to `*func`, plus the estimated size of the table in pages
- * should br stored to `*totalpages`.
- */
-static inline void
-table_relation_analyze(Relation relation, AcquireSampleRowsFunc *func,
-					   BlockNumber *totalpages, BufferAccessStrategy bstrategy)
-{
-	relation->rd_tableam->relation_analyze(relation, func,
-										   totalpages, bstrategy);
-}
 
 /* ----------------------------------------------------------------------------
  * Miscellaneous functionality
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 12a03abb75a..759f9a87d38 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -21,11 +21,9 @@
 #include "catalog/pg_class.h"
 #include "catalog/pg_statistic.h"
 #include "catalog/pg_type.h"
-#include "executor/tuptable.h"
 #include "parser/parse_node.h"
 #include "storage/buf.h"
 #include "storage/lock.h"
-#include "storage/read_stream.h"
 #include "utils/relcache.h"
 
 /*
@@ -178,21 +176,6 @@ typedef struct VacAttrStats
 	int			rowstride;
 } VacAttrStats;
 
-/*
- * AcquireSampleRowsFunc - a function for the sampling statistics collection.
- *
- * A random sample of up to `targrows` rows should be collected from the
- * table and stored into the caller-provided `rows` array.  The actual number
- * of rows collected must be returned.  In addition, a function should store
- * estimates of the total numbers of live and dead rows in the table into the
- * output parameters `*totalrows` and `*totaldeadrows1.  (Set `*totaldeadrows`
- * to zero if the storage does not have any concept of dead rows.)
- */
-typedef int (*AcquireSampleRowsFunc) (Relation relation, int elevel,
-									  HeapTuple *rows, int targrows,
-									  double *totalrows,
-									  double *totaldeadrows);
-
 /* flag bits for VacuumParams->options */
 #define VACOPT_VACUUM 0x01		/* do VACUUM */
 #define VACOPT_ANALYZE 0x02		/* do ANALYZE */
@@ -392,61 +375,9 @@ extern void parallel_vacuum_cleanup_all_indexes(ParallelVacuumState *pvs,
 extern void parallel_vacuum_main(dsm_segment *seg, shm_toc *toc);
 
 /* in commands/analyze.c */
-
-struct TableScanDescData;
-
-
-/*
- * A callback to prepare to analyze block from `stream` of `scan`. The scan
- * has been started with table_beginscan_analyze().
- *
- * The callback may acquire resources like locks that are held until
- * ScanAnalyzeNextTupleFunc returns false. In some cases it could be
- * useful to hold a lock until all tuples in a block have been analyzed by
- * ScanAnalyzeNextTupleFunc.
- *
- * The callback can return false if the block is not suitable for
- * sampling, e.g. because it's a metapage that could never contain tuples.
- *
- * This is primarily suited for block-based AMs. It's not clear what a
- * good interface for non block-based AMs would be, so there isn't one
- * yet and sampling using a custom implementation of acquire_sample_rows
- * may be preferred.
- */
-typedef bool (*ScanAnalyzeNextBlockFunc) (struct TableScanDescData *scan,
-										  ReadStream *stream);
-
-/*
- * A callback to iterate over tuples in the block selected with
- * ScanAnalyzeNextBlockFunc (which needs to have returned true, and
- * this routine may not have returned false for the same block before). If
- * a tuple that's suitable for sampling is found, true is returned and a
- * tuple is stored in `slot`.
- *
- * *liverows and *deadrows are incremented according to the encountered
- * tuples.
- *
- * Not every AM might have a meaningful concept of dead rows, in which
- * case it's OK to not increment *deadrows - but note that that may
- * influence autovacuum scheduling (see comment for relation_vacuum
- * callback).
- */
-typedef bool (*ScanAnalyzeNextTupleFunc) (struct TableScanDescData *scan,
-										  TransactionId OldestXmin,
-										  double *liverows,
-										  double *deadrows,
-										  TupleTableSlot *slot);
-
 extern void analyze_rel(Oid relid, RangeVar *relation,
 						VacuumParams *params, List *va_cols, bool in_outer_xact,
 						BufferAccessStrategy bstrategy);
-extern void block_level_table_analyze(Relation relation,
-									  AcquireSampleRowsFunc *func,
-									  BlockNumber *totalpages,
-									  BufferAccessStrategy bstrategy,
-									  ScanAnalyzeNextBlockFunc scan_analyze_next_block_cb,
-									  ScanAnalyzeNextTupleFunc scan_analyze_next_tuple_cb);
-
 extern bool std_typanalyze(VacAttrStats *stats);
 
 /* in utils/misc/sampling.c --- duplicate of declarations in utils/sampling.h */
diff --git a/src/include/foreign/fdwapi.h b/src/include/foreign/fdwapi.h
index 0968e0a01ec..7f0475d2fa7 100644
--- a/src/include/foreign/fdwapi.h
+++ b/src/include/foreign/fdwapi.h
@@ -13,7 +13,6 @@
 #define FDWAPI_H
 
 #include "access/parallel.h"
-#include "commands/vacuum.h"
 #include "nodes/execnodes.h"
 #include "nodes/pathnodes.h"
 
@@ -149,8 +148,13 @@ typedef void (*ExplainForeignModify_function) (ModifyTableState *mtstate,
 typedef void (*ExplainDirectModify_function) (ForeignScanState *node,
 											  struct ExplainState *es);
 
+typedef int (*AcquireSampleRowsFunc) (Relation relation, int elevel,
+									  HeapTuple *rows, int targrows,
+									  double *totalrows,
+									  double *totaldeadrows);
+
 typedef bool (*AnalyzeForeignTable_function) (Relation relation,
-											  AcquireSampleRowsFunc *func,
+											  AcquireSampleRowsFunc * func,
 											  BlockNumber *totalpages);
 
 typedef List *(*ImportForeignSchema_function) (ImportForeignSchemaStmt *stmt,
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index c83417ce9d2..50aad8f39e5 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -22,7 +22,6 @@ AclItem
 AclMaskHow
 AclMode
 AclResult
-AcquireSampleRowsFunc
 ActionList
 ActiveSnapshotElt
 AddForeignUpdateTargets_function
@@ -2533,8 +2532,6 @@ ScalarIOData
 ScalarItem
 ScalarMCVItem
 Scan
-ScanAnalyzeNextBlockFunc
-ScanAnalyzeNextTupleFunc
 ScanDirection
 ScanKey
 ScanKeyData
-- 
2.39.3 (Apple Git-145)

Reply via email to