Re: reload-through-the-top-parent switch the partition table

On Thu, Aug 10, 2017 at 8:26 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Thu, Aug 10, 2017 at 3:47 AM, Rushabh Lathia
> <rushabh(dot)lathia(at)gmail(dot)com> wrote:
> >> (1) seems like a pretty arbitrary restriction, so I don't like that
> >> option. (2) would hurt performance in some use cases. Do we have an
> >> option (3)?
> >
> > How about protecting option 2) with the load-via-partition-root
> protection.
> > Means
> > load the parents information even dump is not set only when
> > load-via-partition-root
> > & ispartition.
> >
> > This won't hurt performance for the normal cases.
>
> Yes, that seems like the right approach.
>
> + Dump data via the top-most partitioned table (rather than
> partition
> + table) when dumping data for the partition table.
>
> I think we should phrase this a bit more clearly, something like this:
> When dumping a COPY or INSERT statement for a partitioned table,
> target the root of the partitioning hierarchy which contains it rather
> than the partition itself. This may be useful when reloading data on
> a server where rows do not always fall into the same partitions as
> they did on the original server. This could happen, for example, if
> the partitioning column is of type text and the two system have
> different definitions of the collation used to partition the data.
>
>
Done.

> + printf(_(" --load-via-partition-root load partition table via
> the root relation\n"));
>
> "relation" seems odd to me here. root table?
>
>
Done.

> /* Don't bother computing anything for non-target tables, either
> */
> if (!tblinfo[i].dobj.dump)
> + {
> + /*
> + * Load the parents information for the partition table when
> + * the load-via-partition-root option is set. As we need the
> + * parents information to get the partition root.
> + */
> + if (dopt->load_via_partition_root &&
> + tblinfo[i].ispartition)
> + findParentsByOid(&tblinfo[i], inhinfo, numInherits);
> continue;
> + }
>
> Duplicating the call to findParentsByOid seems less then ideal. How
> about doing something like this:
>
> if (tblinfo[i].dobj.dump)
> {
> find_parents = true;
> mark_parents = true;
> }
> else if (dopt->load_via_partition_root && tblinfo[i].ispartition)
> find_parents = true;
>
> if (find_parents)
> findParentsByOid(&tblinfo[i], inhinfo, numInherits);
>
> etc.
>
>
Done changes to avoid duplicate call to findParentsByOid().

> The comments for this function also need some work - e.g. the function
> header comment deserves some kind of update for these changes.
>
>
Done.

> +static TableInfo *
> +getRootTableInfo(TableInfo *tbinfo)
> +{
> + Assert(tbinfo->ispartition);
> + Assert(tbinfo->numParents == 1);
> + if (tbinfo->parents[0]->ispartition)
> + return getRootTableInfo(tbinfo->parents[0]);
> +
> + return tbinfo->parents[0];
> +}
>
> This code should iterate, not recurse, to avoid any risk of blowing
> out the stack.
>
>
Done.

Please find attach patch with the changes.

Thanks,
Rushabh Lathia
www.EnterpriseDB.com