On Thu, Jan 24, 2019 at 9:46 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Jan 24, 2019 at 3:39 AM John Naylor <john.naylor@2ndquadrant.com> wrote:
> >
Few comments related to pg_upgrade patch:
1.
+ if ((maps[mapnum].relkind != RELKIND_RELATION &&
+ maps[mapnum].relkind != RELKIND_TOASTVALUE) ||
+ first_seg_size > HEAP_FSM_CREATION_THRESHOLD * BLCKSZ ||
+ GET_MAJOR_VERSION(new_cluster.major_version) <= 1100)
+ (void) transfer_relfile(&maps[mapnum], "_fsm", vm_must_add_frozenbit);
I think this check will needlessly be performed for future versions as
well, say when wants to upgrade from PG12 to PG13. That might not
create any problem, but let's try to be more precise. Can you try to
rewrite this check? You might want to encapsulate it inside a
function. I have thought of doing something similar to what we do for
vm, see checks relate to VISIBILITY_MAP_FROZEN_BIT_CAT_VER, but I
guess for this patch it is not important to check catalog version as
even if someone tries to upgrade to the same version.
2.
transfer_relfile()
{
..
- /* Is it an extent, fsm, or vm file? */
- if (type_suffix[0] != '\0' || segno != 0)
+ /* Did file open fail? */
+ if (stat(old_file, &statbuf) != 0)
..
}
So from now onwards, we will call stat for even 0th segment which
means there is one additional system call for each relation, not sure
if that matters, but I think there is no harm in once testing with a
large number of relations say 10K to 50K relations which have FSM.
The other alternative is we can fetch pg_class.relpages and rely on
that to take this decision, but again if that is not updated, we might
take the wrong decision.
Anyone else has any thoughts on this point?
3.
-static void
+static Size
transfer_relfile(FileNameMap *map, const char *type_suffix, bool
vm_must_add_frozenbit)
If we decide to go with the approach proposed by you, we should add
some comments atop this function for return value change?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com