Re: WIP: Avoid creation of the free space map for small tables
От | Amit Kapila |
---|---|
Тема | Re: WIP: Avoid creation of the free space map for small tables |
Дата | |
Msg-id | CAA4eK1K5_P53JzWO6Wq81MbQzK_vRuXA9fh3htAi0B+TpC0ZGw@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: WIP: Avoid creation of the free space map for small tables (John Naylor <john.naylor@2ndquadrant.com>) |
Ответы |
Re: WIP: Avoid creation of the free space map for small tables
Re: WIP: Avoid creation of the free space map for small tables Re: WIP: Avoid creation of the free space map for small tables |
Список | pgsql-hackers |
On Mon, Jan 28, 2019 at 2:33 AM John Naylor <john.naylor@2ndquadrant.com> wrote: > > On Sat, Jan 26, 2019 at 2:14 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > I think there is some value in using the information from > > this function to skip fsm files, but the code doesn't appear to fit > > well, how about moving this check to new function > > new_cluster_needs_fsm()? > > For v21, new_cluster_needs_fsm() has all responsibility for obtaining > the info it needs. I think this is much cleaner, > Right, now the code looks much better. > but there is a small > bit of code duplication since it now has to form the file name. One > thing we could do is form the the base old/new file names in > transfer_single_new_db() and pass those to transfer_relfile(), which > will only add suffixes and segment numbers. We could then pass the > base old file name to new_cluster_needs_fsm() and use it as is. Not > sure if that's worthwhile, though. > I don't think it is worth. Few minor comments: 1. warning C4715: 'new_cluster_needs_fsm': not all control paths return a value Getting this new warning in the patch. 2. + + /* Transfer any VM files if we can trust their contents. */ if (vm_crashsafe_match) 3. Can we add a note about this in the Notes section of pg_upgrade documentation [1]? This comment line doesn't seem to be related to this patch. If so, I think we can avoid having any additional change which is not related to the functionality of this patch. Feel free to submit it separately, if you think it is an improvement. Have you done any performance testing of this patch? I mean to say now that we added a new stat call for each table, we should see if that has any impact. Ideally, that should be compensated by the fact that we are now not transferring *fsm files for small relations. How about constructing a test where all relations are greater than 4 pages and then try to upgrade them. We can check for a cluster with a different number of relations say 10K, 20K, 50K, 100K. In general, the patch looks okay to me. I would like to know if anybody else has any opinion whether pg_upgrade should skip transferring fsm files for small relations or not? I think both me and John thinks that it is good to have feature and now that patch turns out to be simpler, I feel we can go ahead with this optimization in pg_upgrade. [1] - https://www.postgresql.org/docs/devel/pgupgrade.html -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
В списке pgsql-hackers по дате отправления:
Предыдущее
От: David RowleyДата:
Сообщение: Re: Update does not move row across foreign partitions in v11